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

Front-load handling of callback responses from the client. #79

Merged
merged 7 commits into from
Feb 8, 2022

Conversation

creachadair
Copy link
Owner

Previously, a notification handler that issues a call back to the client could
block delivery of the reply for its own callback: The barrier we use to
preserve issue order means another batch cannot be issued to the dispatcher
until all previously-issued notifications have completed.

To prevent the handler from deadlocking itself in this case, filter out
response messages from the client when the input is received, rather than
enqueuing them with the handlers. This basically just moves the existing logic
earlier in the transaction, but it means replies can be delivered even if the
barrier is active.

Add regression test for deadlock bug.

Fixes #78.

creachadair and others added 2 commits February 7, 2022 12:33
Previously, a notification handler that issues a call back to the client could
block delivery of the reply for its own callback: The barrier we use to
preserve issue order means another batch cannot be issued to the dispatcher
until all previously-issued notifications have completed.

To prevent the handler from deadlocking itself in this case, filter out
response messages from the client when the input is received, rather than
enqueuing them with the handlers. This basically just moves the existing logic
earlier in the transaction, but it means replies can be delivered even if the
barrier is active.

Fixes #78.
@creachadair creachadair added the bug Something isn't working label Feb 7, 2022
@creachadair creachadair self-assigned this Feb 7, 2022
Copy link
Contributor

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

I can confirm this fixes the issue and the solution looks good to me.

Previous test cases that were hanging downstream are now passing too and the progress reporting feature (relying on this fix) seems to work as intended!

Thank you for the quick turnaround.

@creachadair creachadair merged commit b6901af into default Feb 8, 2022
@creachadair creachadair deleted the mjf/callback-responses branch February 8, 2022 21:06
creachadair added a commit that referenced this pull request Feb 8, 2022
Fixes:
- 1a458a9 server: handle duplicate request IDs within a single batch (#77)
- b6901af Front-load handling of callback responses from the client. (#79)

Cleanup:
- 9cb4b74 Document method origin in ParseQuery.
- 8b046b1 Add more goroutine leak checks.
- f2aaf13 Use code strings consistently.
@creachadair
Copy link
Owner Author

This fix is now releaseed at v0.35.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants