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

Double Terminate Crash #101

Merged
merged 1 commit into from
Jul 3, 2023
Merged

Double Terminate Crash #101

merged 1 commit into from
Jul 3, 2023

Conversation

bretambrose
Copy link
Contributor

@bretambrose bretambrose commented Jul 2, 2023

Ensure that the continuation completion callbacks and its decref only get performed once no matter what the situation

Greengrass has a high-level repro and the logs show the completion callback and the decref inside occuring twice for the same continuation, once due to terminate stream flags on the outbound client socket write callback and once due to terminate stream flags received in the meantime from the server's response.

I was not able to come up with a test with the existing framework that triggered this condition, however.

Solution uses an atomic because it's not clear we can guarantee that the completion callback always gets invoked on the event loop thread.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

… get performed once no matter what the situation
@bretambrose bretambrose closed this Jul 3, 2023
@bretambrose bretambrose reopened this Jul 3, 2023
@bretambrose
Copy link
Contributor Author

bretambrose commented Jul 3, 2023

Per Mike's feedback on the non-atomic version, the non-atomic version doesn't prevent queued completions (via a socket write task) from calling complete after the connection's continuation table has been "cleared."

@bretambrose bretambrose merged commit ec1716c into main Jul 3, 2023
30 checks passed
@bretambrose bretambrose deleted the DoubleTerminateCrash branch July 3, 2023 19:08
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