-
Notifications
You must be signed in to change notification settings - Fork 254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SSE now works with preceding channel handlers such as SSL #394
Conversation
@johanhaleby thanks for the PR! Looking at the code now, it seems that the |
I discovered this when I was working with https support in Turbine (here's the link to that pull request). After a lot of investigation and trail-and-error I found that this was the problem. Unfortunately I couldn't write an automated test case to verify it. I discovered it while trying to combine SSE and SSL in Turbine. The changes made here and in Turbine made it work for our Hystrix stream that is using https. |
ok @johanhaleby can you verify if turbine streams work over SSL when instead of the code in this PR: if (!HttpHeaders.isTransferEncodingChunked((HttpResponse) msg)) {
pipeline.addBefore(NAME, SSE_DECODER_HANDLER_NAME, new ServerSentEventDecoder());
/*
* If there are buffered messages in the previous handler at the time this message is read, we would
* not be able to convert the content into an SseEvent. For this reason, we also add the decoder after
* this handler, so that we can handle the buffered messages.
* See the class level javadoc for more details.
*/
pipeline.addAfter(NAME, SSE_DECODER_POST_INBOUND_HANDLER, new ServerSentEventDecoder());
} else {
pipeline.addAfter(NAME, SSE_DECODER_HANDLER_NAME, new ServerSentEventDecoder());
} you change it to: pipeline.addAfter(NAME, SSE_DECODER_HANDLER_NAME, new ServerSentEventDecoder()); i.e. remove the check I did verify that SSE over plain HTTP works with this change. If this does work then I would like to instead make this change and remove the redundant check. |
@NiteshKant It seems to work without the if-statement. FYI this is our Nginx settings in production on the server I'm testing against:
I.e. transfer encoding is indeed off. |
I've updated the pull request and removed the if-statement |
SSE now works with preceding channel handlers such as SSL
Thanks @johanhaleby ! |
Previously it was not possible to connect to a SSE server that also used SSL (https). The reason is that
SSEInboundHandler
/SseChannelHandler
added theServerSentEventDecoder
before the SSL decoder handler in the pipeline chain which madeServerSentEventDecoder
operate on encrypted data. This pull requests corrects this by not adding theServerSentEventDecoder
first in the pipeline.