Denormalize "Code", slight simplification of HTTP interface #224
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…olClient.stream 1. This denormalizes the presence of code. It was previously present both on StreamResult.Complete and ResponseMessage, even though it was redundant with the code on any non-null ConnectException. 2. Removes Code.OK. It is now just an error code. An "OK" result is easily distinguished by the lack of an exception. 3. The HTTPClientInterface was previously responsible for translating HTTP status -> Code and the HTTPResponse reported back the status in an optional "trace info" field. But translating status to Code should be the responsibility of the protocols, not the HTTP client impl. So simplified by promoting the status code out of trace info and into the HTTPResponse. 4. ProtocolClient.stream used suspendCancellableCoroutine incorrectly. It looks like it was trying to associate the stream with the coroutine, so if the coroutine is cancelled then the stream is closed. But it is only effective *while* that method was running, and it immediately returns a result (no suspensions). After rectifying this, the suspend keyword is no longer actually needed for any of the stream-creation methods as they are all async already.
jhump
changed the title
pkwarren
approved these changes
Feb 22, 2024
|
@jhump Just an FYI, this bit threw me off for a sec as we're upgrading to 0.5.0. It could be valuable to call this out explicitly in the release notes 🙏
|
|
@erawhctim, sorry about that. It is actually mentioned in the release notes (search for "com.connectrpc.Code"). But I didn't do a good job of calling attention to it, so thanks for bringing this up. I've just added a paragraph at the top to make it more clear to users examining the release notes. I've also added a small blurb to the specific bullet that corresponds to this pull request. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sorry that this is kind of a large PR. The main intent was the first bullet, but I tackled some other things that I think are improvements along the way.
StreamResult.CompleteandResponseMessage, even though it was redundant with the code on any non-nullConnectException.Code.OK. It is now just an error code. An "OK" result is easily distinguished by the lack of an exception.HTTPClientInterfacewas previously responsible for translating HTTP status ->Codeand theHTTPResponsereported back the status in an optional "trace info" field. But translating status toCodeshould be the responsibility of the protocol implementations, not the HTTP client. (Though it is still up to the HTTP client to classify exceptions that prevent an HTTP response from being received, as these could be client-implementation-specific exception types). So this change promotes the status code out of trace info and into theHTTPResponse.ProtocolClient.streamusedsuspendCancellableCoroutineincorrectly. It looks like it was trying to associate the stream with the coroutine, so if the coroutine is cancelled then the stream is closed. But it is only effective while that method is running, which is almost none at all since it immediately returns a result (no suspensions).There are some other small clean-ups in here, too, like moving some things around to places that (IMO) make more sense.