-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
Bugfix/unbounded send queue #224
Conversation
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.
This looks like it's broadly on the right track. A couple things in-line.
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.
Some immediate thoughts about the error handling package. Haven't looked through in detail yet, but will try to find time in the next couple days to do a more thorough review.
This is more or less ready for review (modulo the deadlock in This refactor ended up introducing an issue that prompted me to refactor capnp exceptions. Briefly, Note that the use of the standard library Both of the above issues prompted me to make changes to
As a result, I performed the following actions:
Let me know if any of this is unclear or seems harebrained! |
Quoting Louis Thibault (2022-02-22 11:12:45)
This just waits until either c.send or c.recv terminate.
I don't think that will ever happen? in particular, send doesn't return
until the bgctx is canceled, which happens in shutdown, which is after
called *after* this wait.
|
Ok, I think this is now good to go. 🤞
Re bug 2: I took the liberty of simplifying the
|
Ugh ... unit tests are still blocking with the |
Hmm, I'm also seeing quite a few data-races. I'll investigate this later. Input welcome, but I know you're busy so no rush. |
Yeah, definitely need to fix the races. Looks like it's happening in Encoder.Encode, so my first guess is that you missed some spot outside of the send() goroutine that is still directly sending data on the transport. I will have more time to stare at it next week. |
Ok, yeah, grepping for |
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.
Comments in-line. A mix of minor style/docs things and also some major problems/questions.
Remove mentions of sender lock in docstrings.
- Fixes name collision with stdlib 'errors' package - Export error types to allow more idiomatic testing - Support users inspecting exception values
correctly handle context.Canceled.
fceace2
to
6388313
Compare
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.
Some minor comments in-line (including bumps on a few things I'd asked about last time). Mostly looks good though.
Fixes #190 and #194.