ensure handshake is performed

[yuya373]

emacs26 pass :nowait to open-gnutls-stream but emacs25 doesn't. emacs-26 branch on github: https://github.com/emacs-mirror/emacs/blob/emacs-26/lisp/net/network-stream.el#L381 emacs-25 branch on github: https://github.com/emacs-mirror/emacs/blob/emacs-25/lisp/net/network-stream.el#L354

And other open stream functions which is used when gnutls is not available seems don't support :nowait arguments, so we should check process-status is open or not instead of passed argument nowait.

[ahyatt]

Thanks for the change! Your reasoning is valid, I think. But I'm a little worried that this will mean that sometimes when clients are not setting nowait, then connection won't be guaranteed to be usable. I think we should either wait (perhaps in a retry loop?) or error out if the connection isn't open. What do you think?

[yuya373]

I’m not c expert, but look through underlying function make-network-process, Emacs raise error when failed to connect. So setting nowait to nil and call make-network-process and return without error, connection is open.

https://github.com/emacs-mirror/emacs/blob/e20d7381ee85611f9e1d1e6bef4fe2d7e2ae7780/src/process.c#L4179 https://github.com/emacs-mirror/emacs/blob/e20d7381ee85611f9e1d1e6bef4fe2d7e2ae7780/src/process.c#L4196 https://github.com/emacs-mirror/emacs/blob/e20d7381ee85611f9e1d1e6bef4fe2d7e2ae7780/src/process.c#L3598 https://github.com/emacs-mirror/emacs/blob/e20d7381ee85611f9e1d1e6bef4fe2d7e2ae7780/src/process.c#L3524 https://github.com/emacs-mirror/emacs/blob/e20d7381ee85611f9e1d1e6bef4fe2d7e2ae7780/src/process.c#L3528

[ahyatt]

Thanks for the reply. I looked up the possible process statuses, and it would either have to be open, or closed. Either way should work here.

However, another thought occurred to me: how do we know that if nowait is true we don't try to handshake twice? The process may be open by the time we reach your change, and the sentinel should also get the change notification.

[yuya373]

The process may be open by the time we reach your change, and the sentinel should also get the change notification.

Thanks! I confirmed called twice.

    (set-process-sentinel
     conn
     (websocket-sentinel url conn key protocols extensions custom-header-alist nowait))
    (set-process-query-on-exit-flag conn nil)
    (sleep-for 1)
    (when (eq 'open (process-status conn))
      (websocket-handshake url conn key protocols extensions custom-header-alist))

I add (sleep-for 1) like above, websocket-handshake is called twice.

I fixed to call websocket-handshake once in https://github.com/ahyatt/emacs-websocket/pull/53/commits/38c1f20cb3f98a281bc2673c42a39d6bb0344959

[ahyatt]

Thanks! One additional suggestion: can you rename websocket-handshake to websocket-ensure-handshake, which I think is now a more accurate function name?

[yuya373]

It is more accurate name. I renamed in https://github.com/ahyatt/emacs-websocket/pull/53/commits/607355db44374b4b21619ac40bd76d2b63b5c995. Thanks!