This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Runtime diagnostics for leaked messages in unbounded channels #12971
Runtime diagnostics for leaked messages in unbounded channels #12971
Changes from 14 commits
793a5fd
2b661e2
3144bd5
49720f2
c640cbf
972b452
1bb9602
69f2727
cacf5b9
82e1b1e
75a3389
94b7254
0d527bc
709efba
2706277
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need signed integer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explained in the comment for a struct field: to avoid underflow if, due to the lack of ordering, the counter happens to go < 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internally: yes. But public API doesn't need to be signed integer. This should have been
u32
instead that should still be plenty big for all intents and purposes. Same fortracing_unbounded
function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also wondering how much of a performance difference it actually makes using Relaxed ordering here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not that relaxed ordering makes sense in terms of performance, it's about not having to bother about synchronization of increments/decrements, why signed integer is used. Relaxed ordering is just a consequence of this decision, because more strong guarantees are not needed if we use the unsigned integer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looked into the issue, and as far as I understand it's impossible to guarantee that the counter is never decremented before it's incremented not relying on internals of
mpsc::unbounded()
. Basically, we have the following events:increment
pull
push
decrement
In order for
decrement
to never happen beforeincrement
,push
in thread A must synchronize withpull
in thread B. Note that this is not a synchronization between operations with our atomic counter, but a synchronization ofmpsc::unbounded()
operations we are not in control of. We can try setting the strongest sequentially consistent ordering guarantee forincrement
anddecrement
, but for this to workpush
andpull
must also be sequentially consistent operations, what is unlikely and cannot be relied on.Please correct me if I'm missing something.
CC @bkchr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use
Acquire
/Release
it should work: https://en.cppreference.com/w/cpp/atomic/memory_orderThe compiler should add some barrier that ensures that reads/writes are not reordered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW @nazar-pc why aren't you just use a channel with a size of 0 and using try_send?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I didn't see
try_send
in there, also it is usually for different access patterns. I'd expect it to still produce a warning regardless.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a PR implementing exact queue size warning (#13117), but I'd like it to be reviewed by somebody with good understanding of concurrency, atomics, and memory order of operations. If you know who to invite for review, please invite them.