fix: Do not publish to connections without context #3873
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a rare case where a closed connection is kept alive while the handling fiber yields, therefore leaving `cc_` (the connection context) pointing to null for other fibers to see. As far as I can see, this can only happen during server shutdown, but there could be other cases that I have missed. The test on its own does _not_ reproduce the crash, however with added `ThisFiber::SleepFor()`s I could reproduce the crash: * Right before `DispatchBrief()` [here](https://github.com/dragonflydb/dragonfly/blob/e3214cb603fce42c8bb52a2c220c3115b7f8cecf/src/server/channel_store.cc#L154) * Right after connection context `reset()` [here](https://github.com/dragonflydb/dragonfly/blob/2ab480e160dece516b908584f1933b25b760abfb/src/facade/dragonfly_connection.cc#L750) In any case, calling `SendPubMessageAsync()` to a connection where `cc_` is null is a bug, and we fix that here.
romange
previously approved these changes
Oct 6, 2024
src/server/channel_store.cc
Outdated
| @@ -146,7 +146,8 @@ unsigned ChannelStore::SendMessages(std::string_view channel, facade::ArgRange m | |||
| auto it = lower_bound(subscribers_ptr->begin(), subscribers_ptr->end(), idx, | |||
| ChannelStore::Subscriber::ByThreadId); | |||
| while (it != subscribers_ptr->end() && it->Thread() == idx) { | |||
| if (auto* ptr = it->Get(); ptr) | |||
| // A connection might have closed or be in the process of closing | |||
There was a problem hiding this comment.
Suggested change
| // A connection might have closed or be in the process of closing | |
| // if ptr->cntx() is null, a connection might have closed or be in the process of closing |
kostasrim
reviewed
Oct 7, 2024
| @@ -146,7 +146,8 @@ unsigned ChannelStore::SendMessages(std::string_view channel, facade::ArgRange m | |||
| auto it = lower_bound(subscribers_ptr->begin(), subscribers_ptr->end(), idx, | |||
| ChannelStore::Subscriber::ByThreadId); | |||
| while (it != subscribers_ptr->end() && it->Thread() == idx) { | |||
| if (auto* ptr = it->Get(); ptr) | |||
| // if ptr->cntx() is null, a connection might have closed or be in the process of closing | |||
There was a problem hiding this comment.
Can it be not null yet the state is_closing ? If thast's the case shouldn't we also cover this here ?
There was a problem hiding this comment.
Sorry, I'm not sure which is_closing state you are referring to, there doesn't seem to be such a state in facade::Connection nor in util::Connection?
There was a problem hiding this comment.
when we close a connection we set the member field of conn_context called conn_closing. I am asking if we also need to poll that here?
There was a problem hiding this comment.
ptr->cntx()->conn_closing
There was a problem hiding this comment.
Gotcha, conn_closing case is already supported and handled inside SendAsync, so we're good with that regards
adiholden
approved these changes
Oct 8, 2024
chakaz
added a commit
that referenced
this pull request
Nov 14, 2024
This is a rare condition, which we know can happen during shutdown (see [here](#3873 (comment)))
chakaz
added a commit
that referenced
this pull request
Nov 14, 2024
* fix: Do not use `cc_` in connection if it's null This is a rare condition, which we know can happen during shutdown (see [here](#3873 (comment))) * add comment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a rare case where a closed connection is kept alive while the handling fiber yields, therefore leaving
cc_(the connection context) pointing to null for other fibers to see.As far as I can see, this can only happen during server shutdown, but there could be other cases that I have missed.
The test on its own does not reproduce the crash, however with added
ThisFiber::SleepFor()s I could reproduce the crash:DispatchBrief()herereset()hereIn any case, calling
SendPubMessageAsync()to a connection wherecc_is null is a bug, and we fix that here.