More request information in Interceptor.WrapStreamContext() #296
Labels
enhancement
New feature or request
Comments
|
Same here! Unary interceptors work but couldn't find a way to do that with streaming interceptors. |
|
Is the context passed in WrapStreamContext(), WrapStreamSender(), and WrapStreamReceiver() interface methods request scoped or global context? |
|
Hi @DeepankarJha! The context is request-scoped. I'm on vacation for a few days, but I'll respond to this issue in detail early next week. |
@akshayjshah any updates on this? |
|
I'm exploring a few different options for updating the interceptor interface. I think I've found something promising, but I'd like to make sure we've gotten it right before making a breaking change. I'll have a PR open next week for you and @anzboi to look at. |
|
Any updates? |
|
Still pushing on this; a few internal things took my focus for a bit. This turns out to be a fairly invasive change to the internals of the package :/ Sorry, please bear with me. |
akshayjshah
added a commit
that referenced
this issue
Jul 9, 2022
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
added a commit
that referenced
this issue
Jul 9, 2022
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
added a commit
that referenced
this issue
Jul 12, 2022
* 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]>
|
We'll ship #316 as part of the next release, probably by end of week. I'd like to ship an interceptor to recover panics as part of that release - it's broadly useful and a good example of implementing the |
Your timing with this comment couldn't have been better - I was just about to start trying to figure out a recovery interceptor using the old API when I got the email notification about your comment. I'd offer to write up the example middleware, but I don't know enough about the streaming API yet to do efficiently, so I'll wait for your example. Thank you for making these changes to the interceptor API, it looks like they will be very useful. |
|
Perfect! Lucky coincidence :)
…On Tue, Jul 12, 2022 at 2:15 PM MK ***@***.***> wrote:
I'd like to ship an interceptor to recover panics as part of that release
Your timing with this comment couldn't have been better - I was just about
to start trying to figure out a recovery interceptor using the old API when
I got the email notification about your comment. I'd offer to write up the
example middleware, but I don't know enough about the streaming API yet to
do efficiently, so I'll wait for your example.
Thank you for making these changes to the interceptor API, it looks like
they will be very useful.
—
Reply to this email directly, view it on GitHub
<https://github.com/bufbuild/connect-go/issues/296#issuecomment-1182507730>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHNP5T5UEU2O5KD4OZY56LVTXN77ANCNFSM5YWJ5VAA>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
akshayjshah
added a commit
that referenced
this issue
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
Currently there is no way to enrich a stream context with information about the request. The
WrapStreamContext()method only accepts a parent context and does not give us any information about the request.In particular, we need access to the endpoint spec and request headers.
Our primary use case for this is authentication, where we would like to be able to apply some generic token parsing and validation logic to all requests, and enrich context with authentication info.
The text was updated successfully, but these errors were encountered: