Bugfix/unbounded send queue

[lthibault]

Fixes #190 and #194.

[zenhack]

This looks like it's broadly on the right track. A couple things in-line.

[zenhack]

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.

[lthibault]

This is more or less ready for review (modulo the deadlock in TestRecvAbort).

This refactor ended up introducing an issue that prompted me to refactor capnp exceptions. Briefly, context.Canceled is returned from send operations during shutdown, causing a number of tests to fail because they were expecting nil errors. The fix was to wrap errors with fmt.Errorf and the %w verb instead of flattening everything to strings with %v or err.Error(), allowing me to test using errors.Is from the standard library.

Note that the use of the standard library errors package introduced a name collision with our own internal/errors package, which has already been an annoyance in the past. More generally, I've found it difficult to inspect error values returned by the RPC code due to this "flattening" effect, and have been thinking quite a bit about how this could be improved.

Both of the above issues prompted me to make changes to internal/errors, with the goal of:

1. Facilitating the inspection of error values using standard libarary errors.Is and errors.As.
2. Exporting an Exception type to users.
3. Eventually supporting a richer error system (e.g. providing a SendError type that wraps a more specific error type, itself a capnp Exception).

As a result, I performed the following actions:

1. Renamed the errors package to exc (short for "exception")
2. Renamed exc.Error to Exception
3. Implemented an Annotator type to facilitate package-level annotations (e.g. with the "rpc" prefix)
4. Refactored error handling to make use of the new Annotator type
5. Added an exc.IsType function that asserts exception types in a manner analogous to errors.Is. Although I ended up not using it in this PR, I nevertheless opted to keep it in the code base as it will be useful for future improvements.

Let me know if any of this is unclear or seems harebrained!

[zenhack]

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.

[lthibault]

Ok, I think this is now good to go. 🤞

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.

bgctx expires when either of the errgroup's goroutines returns a non-nil error. There were actually two bugs here:

1. I mistakenly thought bgctx would expire on nil errors as well.
2. bgctx is always expired when we enter shutdown(), so we need an additional flag to suppress duplicate calls.

Re bug 2: I took the liberty of simplifying the Close() and shutdown() logic a bit, as well as apply some opinionated readability improvements. In particular:

• Close no longer returns ExcAlreadyClosed. Repeated calls to Close() are idempotent, and will simply return nil.
• The c.closed flag was renamed to closing and is used to detect duplicate calls to shutdown.

[lthibault]

Ugh ... unit tests are still blocking with the -race flag. Going to investigate a bit, but they definitely pass on my machine (go1.17.7).

[lthibault]

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.

[zenhack]

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.

[zenhack]

Ok, yeah, grepping for c.transport.NewMessage comes up with a bunch of places where we're sending a message outside of the send goroutine. So those will need to be fixed to delegate to the send goroutine.

[zenhack]

Comments in-line. A mix of minor style/docs things and also some major problems/questions.

[zenhack]

Some minor comments in-line (including bumps on a few things I'd asked about last time). Mostly looks good though.