Rework streaming interceptors #316
Merged
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
Streaming interceptors aren't very useful in their current state. Rather
than wrapping user code, they wrap the the networking primitives used by
the user's code. This is a subtle distinction, but it makes it
impossible to write a panic-recovering interceptor. It also makes common
server-side interceptors (e.g., authentication) needlessly difficult to
write.
This commit reworks the Sender and Receiver interfaces into a ClientConn
and HandlerConn. Just as unary interceptors wrap a UnaryFunc, streaming
interceptors now wrap ClientConnFuncs and HandlerConnFuncs. This ends up
being quite a bit easier to understand.
Connect's internals change a lot, but the user-visible changes are
modest. Today, we have:
```go
type Sender interface {
Spec() Spec
Send(any) error
Header() http.Header
Trailer() (http.Header, bool)
Close(error) error
}
type Receiver interface {
Spec() Spec
Receive(any) error
Header() http.Header
Trailer() (http.Header, bool)
Close() error
}
type Interceptor interface {
WrapUnary(UnaryFunc) UnaryFunc
WrapStreamContext(context.Context) context.Context
WrapStreamSender(context.Context, Sender) Sender
WrapStreamReceiver(context.Context, Receiver) Receiver
}
```
With this commit, we change to:
```go
type ClientConn interface {
Spec() Spec
Send(any) error
RequestHeader() http.Header
CloseRequest() error
Receive(any) error
ResponseHeader() http.Header
ResponseTrailer() http.Header
CloseResponse() error
}
type HandlerConn interface {
Spec() Spec
Receive(any) error
RequestHeader() http.Header
Send(any) error
ResponseHeader() http.Header
ResponseTrailer() http.Header
Close(error) error
}
type ClientConnFunc func(Spec, http.Header) ClientConn
type HandlerConnFunc func(context.Context, HandlerConn) error
type Interceptor interface {
WrapUnary(UnaryFunc) UnaryFunc
WrapStreamingClient(ClientConnFunc) ClientConnFunc
WrapStreamingHandler(HandlerConnFunc) HandlerConnFunc
}
```
Fixes #296.
akshayjshah
requested review from
pkwarren,
amckinney,
doriable and
bufdev
and removed request for
doriable
pkwarren
reviewed
Jul 11, 2022
| _ = sender.Close(sender.Send(response.Any())) | ||
| mergeHeaders(conn.ResponseHeader(), response.Header()) | ||
| mergeHeaders(conn.ResponseTrailer(), response.Trailer()) | ||
| return conn.Send(response.Any()) |
There was a problem hiding this comment.
We used to call sender.Close(...) here - is that now replaced by: https://github.com/bufbuild/connect-go/pull/316/files#diff-0eb779b9e49d8e44b0f36923fdb8d87d5ee024f886eefc45deec4ec88380a087R210
Co-authored-by: Philip K. Warren <[email protected]>
Co-authored-by: Philip K. Warren <[email protected]>
Co-authored-by: Philip K. Warren <[email protected]>
Co-authored-by: Philip K. Warren <[email protected]>
|
I'm not going to go through all the code, but can we talk briefly offline about interfaces? Want to make sure I understand this. |
bufdev
approved these changes
Jul 12, 2022
There was a problem hiding this comment.
@akshayjshah and I talked offline, going to make a slight naming change, otherwise LGTM
Rename to make it clearer that the new Conn interfaces are only relevant to users of streaming RPCs.
pkwarren
approved these changes
Jul 12, 2022
akshayjshah
added a commit
that referenced
this pull request
Jul 26, 2023
* Rework streaming interceptors
Streaming interceptors aren't very useful in their current state. Rather
than wrapping user code, they wrap the the networking primitives used by
the user's code. This is a subtle distinction, but it makes it
impossible to write a panic-recovering interceptor. It also makes common
server-side interceptors (e.g., authentication) needlessly difficult to
write.
This commit reworks the Sender and Receiver interfaces into a ClientConn
and HandlerConn. Just as unary interceptors wrap a UnaryFunc, streaming
interceptors now wrap ClientConnFuncs and HandlerConnFuncs. This ends up
being quite a bit easier to understand.
Connect's internals change a lot, but the user-visible changes are
modest. Today, we have:
```go
type Sender interface {
Spec() Spec
Send(any) error
Header() http.Header
Trailer() (http.Header, bool)
Close(error) error
}
type Receiver interface {
Spec() Spec
Receive(any) error
Header() http.Header
Trailer() (http.Header, bool)
Close() error
}
type Interceptor interface {
WrapUnary(UnaryFunc) UnaryFunc
WrapStreamContext(context.Context) context.Context
WrapStreamSender(context.Context, Sender) Sender
WrapStreamReceiver(context.Context, Receiver) Receiver
}
```
With this commit, we change to:
```go
type ClientConn interface {
Spec() Spec
Send(any) error
RequestHeader() http.Header
CloseRequest() error
Receive(any) error
ResponseHeader() http.Header
ResponseTrailer() http.Header
CloseResponse() error
}
type HandlerConn interface {
Spec() Spec
Receive(any) error
RequestHeader() http.Header
Send(any) error
ResponseHeader() http.Header
ResponseTrailer() http.Header
Close(error) error
}
type ClientConnFunc func(Spec, http.Header) ClientConn
type HandlerConnFunc func(context.Context, HandlerConn) error
type Interceptor interface {
WrapUnary(UnaryFunc) UnaryFunc
WrapStreamingClient(ClientConnFunc) ClientConnFunc
WrapStreamingHandler(HandlerConnFunc) HandlerConnFunc
}
```
Fixes #296.
* Fix flaky test
* Update client_stream.go
Co-authored-by: Philip K. Warren <[email protected]>
* Update client_stream.go
Co-authored-by: Philip K. Warren <[email protected]>
* Update interceptor_ext_test.go
Co-authored-by: Philip K. Warren <[email protected]>
* Update interceptor_ext_test.go
Co-authored-by: Philip K. Warren <[email protected]>
* Also remove closing parens
* Restructure concurrency GoDoc for ClientConn
* Make missing trailers tests handle timing non-determinism
* Avoid a few map allocations
* Rename to Streaming*Conn
Rename to make it clearer that the new Conn interfaces are only relevant
to users of streaming RPCs.
Co-authored-by: Philip K. Warren <[email protected]>
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.
Streaming interceptors aren't very useful in their current state. Rather
than wrapping user code, they wrap the the networking primitives used by
the user's code. This is a subtle distinction, but it makes it
impossible to write a panic-recovering interceptor. It also makes common
server-side interceptors (e.g., authentication) needlessly difficult to
write.
This commit reworks the Sender and Receiver interfaces into a StreamingClientConn
and StreamingHandlerConn. Just as unary interceptors wrap a UnaryFunc, streaming
interceptors now wrap StreamingClientFuncs and StreamingHandlerFuncs. This ends up
being quite a bit easier to understand.
Connect's internals change a lot, but the user-visible changes are
modest. Today, we have:
With this commit, we change to:
Fixes #296.