Propagate SecurityHandshakeObserver without channel attribute
#1129
Conversation
servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpsProxyTest.java
Outdated
Show resolved
Hide resolved
| if (waitForSslHandshake) { | ||
| if (observer != null) { | ||
| // Notify ConnectionObserverHandler that we are ready to receive SecurityHandshakeObserver instance | ||
| ctx.pipeline().fireUserEventTriggered(WaitingForHandshakeCompletionEvent.INSTANCE); |
There was a problem hiding this comment.
A negative of sending an event is that there is no acknowledgment of receipt which makes debugging harder.
An alternative is to follow what we do for DeferSslHandler => lookup the handler and register interest for receiving the observer. The negative is more public API (internal so arguably low) but gives a more determinsitic behavior.
There was a problem hiding this comment.
Good suggestion! Everything is pkg-private without public exposure 🎉
Motivation: Usage of Channel's attributes increase a shared state of the `Channel`. `SecurityHandshakeObserver` can be propagated without using it. Modifications: - Introduce `WaitingForHandshakeCompletionEvent` that is used to notify `ConnectionObserverHandler` that the last handler is initialized and ready to receive `SecurityHandshakeObserver` instance; - Propagate `SecurityHandshakeObserver` as user event through the pipeline; - Add `SecurityHandshakeObserverTest` to verify that `SecurityHandshakeObserver` is working with different configurations: HTTP/1.1, HTTP/2, ALPN, secure proxy tunnel; - Improve `TestServiceStreaming` to echo content-type header; Result: Less state on the `Channel` instance.
idelpivnitskiy
force-pushed
the
handshake-handle
branch
from
f1d53b4
to
2d2d15e
Compare
| if (securityObserver != null) { | ||
| securityObserver.handshakeComplete(session); | ||
| if (observer != null) { | ||
| observer.handshakeComplete(session); | ||
| } |
There was a problem hiding this comment.
Log at warn if shouldReport is true?
There was a problem hiding this comment.
The warn log is easy to miss. This is not expected to be caught by users, we should provide a guarantee that it never happens. I'm thinking about an assertion, but as we discussed offline we will avoid using null values. Therefore, shouldReport will go away in a follow-up.
There was a problem hiding this comment.
Ok what I meant was that if we expect the observer to be present but it is not; is there something we need to do to bubble it up to the user?
Yes good to guarantee but bugs will make it hard to debug. Perhaps a good item to think about when using NOOP observers.
|
@servicetalk-bot test this please |
Motivation: Usage of Channel's attributes increase a shared state of the `Channel`. `SecurityHandshakeObserver` can be propagated without using it. Modifications: - When `SslHandshakeCompletionEvent` arrives, look up for `ConnectionObserverHandler` to access previously initialized `SecurityHandshakeObserver`; - Add `SecurityHandshakeObserverTest` to verify that `SecurityHandshakeObserver` is working with different configurations: HTTP/1.1, HTTP/2, ALPN, secure proxy tunnel; - Improve `TestServiceStreaming` to echo content-type header; Result: Less state on the `Channel` instance.
Motivation: Usage of Channel's attributes increase a shared state of the `Channel`. `SecurityHandshakeObserver` can be propagated without using it. Modifications: - When `SslHandshakeCompletionEvent` arrives, look up for `ConnectionObserverHandler` to access previously initialized `SecurityHandshakeObserver`; - Add `SecurityHandshakeObserverTest` to verify that `SecurityHandshakeObserver` is working with different configurations: HTTP/1.1, HTTP/2, ALPN, secure proxy tunnel; - Improve `TestServiceStreaming` to echo content-type header; Result: Less state on the `Channel` instance.
Motivation:
Usage of Channel's attributes increase a shared state of the
Channel.SecurityHandshakeObservercan be propagated without using it.Modifications:
SslHandshakeCompletionEventarrives, look up forConnectionObserverHandlerto access previously initializedSecurityHandshakeObserver;SecurityHandshakeObserverTestto verify thatSecurityHandshakeObserveris working with different configurations:HTTP/1.1, HTTP/2, ALPN, secure proxy tunnel;
TestServiceStreamingto echo content-type header;Result:
Less state on the
Channelinstance.