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

cleaning up after a websocket goes aways is not clear #505

Closed
hiredman opened this issue May 6, 2024 · 4 comments
Closed

cleaning up after a websocket goes aways is not clear #505

hiredman opened this issue May 6, 2024 · 4 comments

Comments

@hiredman
Copy link

hiredman commented May 6, 2024

the spec says on-close is guaranteed to be called and can be used for finalization logic.

if you run a little ring-jetty server like:

;; clj -Sdeps '{:deps {ring/ring {:mvn/version "1.12.1"}}}'


(require '[ring.adapter.jetty :refer [run-jetty]]
         '[ring.websocket :as ws]
         '[ring.websocket.protocols :as wsp])

;; patch in logging to see that upgrade-to-websocket is called
(alter-var-root #'ring.adapter.jetty/upgrade-to-websocket
                 (fn [orig]
                   (fn [& args]
                     (prn 'upgrade-to-websocket 'called)
                     (apply orig args))))

(def jetty
  (run-jetty
   (fn [req]
     (prn req)
     {::ws/listener
      (reify wsp/Listener
        (on-open [_ sock]
          (println "on open")
          (ws/send sock "Hello")
          (ws/send sock (java.nio.ByteBuffer/wrap (.getBytes "World"))))
        (on-message [_ sock msg]
          (prn "on close"))
        (on-pong [_ _ _]
          (prn "on pong"))
        (on-error [_ _ _]
          (prn "on error"))
        (on-close [_ _ _ _]
          (prn "on close")))})
   {:port 4395
    :join? false}))

and then hit it with curl like curl -i -N -H "Connection: Upgrade" -H "Upgrade: websocket" http://localhost:4395 you get some interesting behavior:

  1. no valid websocket connection is established of course
  2. on-open is never called
  3. on-close is never called
  4. on-error is never called

With some thought you can come to the conclusion that the fact that on-open was never called is the signal to the handler that the websocket is not valid, but that is not at all clear from the spec, and the spec for on-close doesn't say it is guaranteed to be called if on-open is called, it just says it is guaranteed to be called.

Additionally the jetty websocket impl just forwards the jetty WebSocketListener onClose events to the handler's on-close function, and there is nothing in the docs for WebSocketListener that says onClose is guaranteed to be called. I don't have a reproducer at the moment, but I believer it should be relatively straightforward to replicate, and that it happens a lot in practice, that sometimes when the underlying tcp connection is closed, onClose is never called and there are no exceptions.

@hiredman
Copy link
Author

hiredman commented May 6, 2024

I think I was too hasty in assuming just forwarding jetty's onClose wouldn't result in it always getting called, but hard to tell, I haven't found docs that say it is always called, but I found a stackoverflow post that suggested it was (but now have lost the link to that post)

@weavejester
Copy link
Member

Interesting. I hadn't considered how Jetty would behave under those circumstances. Thank for for bringing it to my attention.

My instinct is to adjust the SPEC and say that on-close is guaranteed to be called if and only if on-open is called, as if the WebSocket is not correctly established and never "opened", it makes no sense for it to then be "closed". This seems the most correct behavior, at least to my mind.

With regard to the Jetty documentation on WebSocketConnectionListener, it says that onWebSocketClose is called when "A Close Event was received." Given that the close status includes events where the WebSocket was closed abnormally - i.e. the TCP connection is terminated without a valid close frame - this implies that once open, a close event is guaranteed. However, I can confirm this on the Jetty mailing list to be sure.

@weavejester
Copy link
Member

The Jetty users mailing list confirms that on-close is guaranteed to be called if on-open is: https://www.eclipse.org/lists/jetty-users/msg10694.html

@hiredman
Copy link
Author

hiredman commented May 7, 2024

interesting, definitely not at all clear to me that "A Close Event was received" in the docs encompasses abnormally termination, but if the jetty mailing list says it is so it must be so. And I didn't realize there was specific reason code to synthesize an onClose call even if no close frame with a reason code came.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants