Skip to content

Conversation

@accelerated
Copy link
Contributor

Two issues:

  • std::deque::size() should be guarded as it's not thread safe. The size() is computed via pointer diff (finish-start) of the queue itself and it returns garbage in a multi-threaded access scenario.
  • The tracker promise may be set multiple times if there are retries. If the produce is asynchronous, the future of this promise is never checked (and as such the promise is never re-created), which means the same promise can be set multiple times, throwing a promise_already_satisfied exception. This in itself is not an error, however it prevents the pending_acks from being properly decremented and the flush eventually blocks forever as the pending_acks mismatch.

@mfontanini please prioritize this if you can. Much appreciated!!

template <typename BufferType, typename Allocator>
size_t BufferedProducer<BufferType, Allocator>::get_buffer_size() const {
return messages_.size() + retry_messages_.size();
int size = 0;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

size_t here

Copy link
Owner

@mfontanini mfontanini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class has become hell, I don't even know what's happening in here anymore. I'll trust you that this fixes a problem and is not potentially hiding another one.

@accelerated
Copy link
Contributor Author

Yes it has gotten a bit out of hand with too many features instead of keeping a strict delineation of functionality and split it into smaller classes. The class API however is quite clean so that's always good. I may revisit the inner implementation the future or perhaps bring back the original design under some other name and keep this one as-is (if you want). Not too many use it so hopefully it's not causing instability elsewhere.
I think however that admin API would have higher priority as future work.

@mfontanini mfontanini merged commit 7d097df into mfontanini:master Feb 4, 2020
@mfontanini
Copy link
Owner

Yeah, the admin API should be the next thing although I probably won't for on that in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants