-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: prevent tx_executor reading from sock if operation is canceled #4704
fix: prevent tx_executor reading from sock if operation is canceled #4704
Conversation
@@ -67,8 +67,6 @@ class ClusterShardMigration { | |||
|
|||
auto tx_data = tx_reader.NextTxData(&reader, cntx); | |||
if (!tx_data) { | |||
in_migration_->ReportError(GenericError("No tx data")); |
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 you need this?
if there is no transaction data you need to report error? if its not running doesnt it mean that error was already reported?
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.
we do report inside tx_reader.NextTxData(&reader, cntx); If the cntx is canceled we don't need report. If we have error it is already reported
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.
So why do you need this?
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 didn't get the question. I'v removed it, because we don't need it
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.
ops sorry my eys are playing with me
break; | ||
|
||
std::optional<TransactionData> tx_data; | ||
while ((tx_data = tx_reader.NextTxData(&reader, cntx))) { |
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 it looks like we did not call the NextTxData if the cntx was closed..
so the problem is in cluster code only where you do not have such if as there was 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.
There is no problem here. just decided to make it shorter
fixes: #4689
problem: we read from the socket after it was shutdown