Conversation
| * @note acquirePubackControl() must be called within the OnPublishReceivedHandler callback. | ||
| * Calling it after the callback returns will return nullptr. | ||
| */ | ||
| class AWS_CRT_CPP_API PubackControlHandle |
There was a problem hiding this comment.
Since this struct doesn't expose any functionality and users work with it via a pointer, probably it's better to move its definition to a source file or a private header.
| public: | ||
| PubackControlHandle() noexcept : m_controlId(0) {} |
There was a problem hiding this comment.
Do we need this public constructor?
And just to reiterate my previous comment: If you move the definition of this class to a source file or a private header, you'll be able to make it just a POD without any constructors or member functions:
struct PubackControlHandle {
uint64_t m_controlId;
};| * sent here. On error, the PUBACK is also auto-sent to avoid losing it. | ||
| */ | ||
| uint64_t pubackControlId = 0; | ||
| auto available = std::make_shared<std::atomic<bool>>(true); |
There was a problem hiding this comment.
The available variable being atomic doesn't handle all multithreading scenarios (and I don't think we have to handle them). Using atomic might give us or users a false confidence that this logic is thread-safe while it's not true. So, maybe just use a plain bool instead and protect only from multiple calls?
There was a problem hiding this comment.
std::shared_ptr should be allocated with Aws::Crt::MakeShared, so crt allocator is used.
You can also get rid of the need to allocate it on heap if replace lambda with a callable struct, which will have this flag as a member field. And this in turn will help with getting rid of std::function member (which might also be allocated on heap). But it's up to you.
| auto handle = | ||
| std::make_shared<PubackControlHandle>(PubackControlHandle(pubackControlId)); | ||
| eventData.acquirePubackControl = [handle, | ||
| available]() -> std::shared_ptr<PubackControlHandle> | ||
| { | ||
| if (!*available) | ||
| { | ||
| return nullptr; | ||
| } | ||
| /* Mark as consumed so a second call returns nullptr */ | ||
| *available = false; | ||
| return handle; | ||
| }; |
There was a problem hiding this comment.
Aws::Crt::ScopedResource (which is just std::unique_ptr with a custom deleter) is more suitable here. You can capture pubackControlId into lambda and create ScopedResource inside.
| bool Mqtt5ClientCore::InvokePuback(const PubackControlHandle &pubackControlHandle) noexcept | ||
| { | ||
| if (m_client == nullptr) | ||
| { | ||
| AWS_LOGF_DEBUG(AWS_LS_MQTT5_CLIENT, "Failed to invoke puback: Mqtt5ClientCore is invalid."); | ||
| return false; | ||
| } | ||
| return aws_mqtt5_client_invoke_puback(m_client, pubackControlHandle.m_controlId, nullptr) == | ||
| AWS_OP_SUCCESS; | ||
| } |
There was a problem hiding this comment.
We added protection from multiple control acquiring. Do we want to add similar protection from multiple invoking?
2 participants
Bind manual PUBACK control
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.