Skip to content
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

Avoid decrementing outstanding requests counter duplicately #935

Merged

Conversation

brfrn169
Copy link
Collaborator

Currently, there is a chance that the outgoing requests counter could be decremented twice when a transaction is being committed or rolled back, and an error occurs simultaneously. To prevent this, this PR introduces a safety measure to ensure the counter is only decremented once. Please take a look!

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

Sorry, I don't understand at what point an error could be thrown that would cause the post process to be executed twice when the transaction is committed or aborted. Would you mind giving more details ?

@brfrn169
Copy link
Collaborator Author

brfrn169 commented Jul 12, 2023

Sorry, I don't understand what point an error could be thrown that would cause the post process to be executed twice when the transaction is committed or aborted. Would you mind giving more details ?

@Torch3333 Apologies for the previous lack of explanation.

Firstly, the preProcessor of TransactionStreamObserver calls gateKeeper.letIn(), and postProcessor calls gateKeeper.letOut():
https://github.com/scalar-labs/scalardb/blob/avoid-decrementing-outstanding-requests-conuter-duplicately/server/src/main/java/com/scalar/db/server/DistributedTransactionService.java#L176-L177

For the pause feature to function correctly, gateKeeper.letIn() and gateKeeper.letOut() must be called an equal number of times.

The callbacks, TransactionStreamObserver.onNext() and TransactionStreamObserver.onError(), are asynchronously called by the gRPC framework. Therefore, I think there's a chance that they are called simultaneously if a message is received (onNext()) and an error occurs (onError()) at the same time. In such a case, the postProcessor could potentially be called twice:
https://github.com/scalar-labs/scalardb/blob/avoid-decrementing-outstanding-requests-conuter-duplicately/server/src/main/java/com/scalar/db/server/DistributedTransactionService.java#L324
https://github.com/scalar-labs/scalardb/blob/avoid-decrementing-outstanding-requests-conuter-duplicately/server/src/main/java/com/scalar/db/server/DistributedTransactionService.java#L452

To address this issue, this PR introduces a safety measure to ensure the counter is only decremented once by using AtomicBoolean:
https://github.com/scalar-labs/scalardb/blob/avoid-decrementing-outstanding-requests-conuter-duplicately/server/src/main/java/com/scalar/db/server/DistributedTransactionService.java#L179

Please let me know if you need further clarification.

@brfrn169 brfrn169 requested a review from Torch3333 July 12, 2023 08:55
@@ -441,7 +449,7 @@ private void cleanUp() {
}
}

postProcessor.run();
postProcess();
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a confirmation. postProcesser shouldn't be called if preProcesser hasn't been called even in this cleanup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct. postProcesser should be called after preProcesser has been called.

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed explanation, I got it now. 🙇
LGTM, thank you!

Copy link
Contributor

@kota2and3kan kota2and3kan left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@brfrn169 brfrn169 merged commit 955abc6 into master Jul 13, 2023
13 checks passed
@brfrn169 brfrn169 deleted the avoid-decrementing-outstanding-requests-conuter-duplicately branch July 13, 2023 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants