Skip to content
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

Handle WebSocket handshake timeouts #422

Open
wants to merge 15 commits into
base: master
from

Conversation

@shilder
Copy link
Contributor

shilder commented Oct 14, 2018

Added new option handshake-timeout for websocket client

Added new option handshake-timeout for websocket client
@shilder

This comment has been minimized.

Copy link
Contributor Author

shilder commented Oct 14, 2018

Related to #346 More like request for discussion. Some opened questions

  1. Is it ok to use somewhat private method (.in time/*clock*) to schedule timeout handler execution ?
  2. There are already couple of specific Exception classes for different timeouts (like ReadTimeoutException and so on). Should i add another class ? Something like WebSocketHandshakeTimeoutException ?
@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Oct 22, 2018

@shilder Regarding WebSocketHandshakeTimeoutException: 👍. It's better to keep all timeouts separate when possible, so the user can distinguish them somehow later. Subclassing works just fine for that.

@@ -609,6 +614,16 @@
([_ ctx]
(let [ch (.channel ctx)]
(reset! in (netty/buffered-source ch (constantly 1) 16))

;; Start handshake timeout timer
(reset! timeout-task

This comment has been minimized.

Copy link
@ztellman

ztellman Oct 30, 2018

Owner

I was trying to figure out why you used the .in accessor here, and realized that manifold.time/in doesn't allow it to be cancelled. That's my bad, I'm going to merge these changes in and then fix the underlying method so we don't have to reach under the covers.

@shilder shilder force-pushed the shilder:ws-handshake-timeout branch from 3f940ab to 075960b Oct 31, 2018
@ztellman

This comment has been minimized.

Copy link
Owner

ztellman commented Feb 1, 2019

@kachayev it seems this no longer merges after I brought in the ping functionality, since you have more familiarity with that would you mind getting it into order?

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 1, 2019

@ztellman Sure, I'll take care.

(fn []
;; if there was no answer after handshake was sent - close channel
(d/error! d (WebSocketHandshakeTimeoutException. "WebSocket handshake timeout"))
(netty/close ctx))))

This comment has been minimized.

Copy link
@kachayev

kachayev Feb 2, 2019

Collaborator

We're not on I/O thread here, so we should probably close the channel, not the context. Right? @ztellman


;; Start handshake timeout timer
(reset! timeout-task
(.in time/*clock*

This comment has been minimized.

Copy link
@kachayev

kachayev Feb 2, 2019

Collaborator

One another thing... Maybe it's better to use EventLoop executor to schedule the task (ctx.executor().schedule) instead of manifold's clock? We can also just trigger user event from the runnable and check if the handshake was complete in :user-triggered. It seems like this approach would leave fewer spaces for race conditions. What do you think? @shilder @ztellman

I still wonder why this functionality is not implemented in Netty, e.g. SslHandler supports handshake timeout as a parameter. I'm going to open an issue to start a discussion there.

This comment has been minimized.

Copy link
@shilder

shilder Feb 2, 2019

Author Contributor

One another thing... Maybe it's better to use EventLoop executor to schedule the task (ctx.executor().schedule) instead of manifold's clock

Yes, we can use EventExecutor and Netty is using it internally in IdleStateHandler to schedule timeout handlers.

And about :user-triggered - yes, it will work, but I'm not sure how it will help with race conditions ? AFAIU Future#cancel provides synchronization point. Or maybe you are talking about situation where context is closed by timeout handler while another thread awaits on Future#cancel ? I'm not a Netty expert, so I'm not sure how it handles serialization of events for context

This comment has been minimized.

Copy link
@kachayev

kachayev Feb 2, 2019

Collaborator

Not only for IdleTimeoutHandler but for all timeout tasks. Future canceling is what I want to avoid here specifically because of a synchronization: it might hurt performance when you introduce sync primitives to the pipeline running on I/O thread.

If you don’t mind, I can update your PR.

This comment has been minimized.

Copy link
@shilder

shilder Feb 2, 2019

Author Contributor

If you don’t mind, I can update your PR.

Sure, go ahead

This comment has been minimized.

Copy link
@kachayev

kachayev Feb 2, 2019

Collaborator

On it!

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 3, 2019

The more I work with this functionality, the more I wonder why don't we use Netty's WebSocket*ProtocolHandler to deal with handshakes? It doesn't support timeouts out of the box (at least for now). Apart from that, it's a well-tested implementation that we can leverage injecting our own handler & realizing deferred with newly created netty/sink on HANDSHAKE_COMPLETE user event triggered by Netty's handler. @ztellman WDYT? Any objections?

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 5, 2019

@shilder I've reimplemented timeouts, please double check on your side. Notable changes:

  1. Avoid .cancel sync on I/O threads
  2. Schedule timeout on I/O threads
  3. Schedule timeout only when the handshake was effectively flushed, not before
  4. Cancel timeout when processing is done, avoid 2 timeouts

I've also added a test case to check that the solution works. Should we do the same for a server handler?

src/aleph/http/client.clj Outdated Show resolved Hide resolved
kachayev added 2 commits Feb 6, 2019
@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Feb 6, 2019

From what I can see, there's no necessary to introduce the same for server's handshake, as it's just about flushing response to the client.

@ztellman Now ready for review/merge.

@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Mar 12, 2019

@ztellman Merged with the latest changes from master, so now it's mergeable.

@shilder I've also moved timeout cancelation to d/finally' instead of d/chain'. Does it make sense?

@ztellman

This comment has been minimized.

Copy link
Owner

ztellman commented Mar 25, 2019

This has conflicts with the other WS changes I merged, but will merge once those are resolved.

@ztellman ztellman added the approved label Mar 25, 2019
@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented Mar 26, 2019

@ztellman Done!

ztellman and others added 2 commits May 14, 2019
@kachayev

This comment has been minimized.

Copy link
Collaborator

kachayev commented May 19, 2019

@ztellman Fixed conflicts here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.