-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Add way to handle WebSocket.onerror events #214
Comments
Hi Daniel, Could you possibly provide some other examples of logic that you'd want to add to the error handler? Retries are already done automatically. |
Hi Peter, Appreciate the quick reply and good point retry is already covered. I'd like to also change the log level and show different UI to the user upon error. Is there a good way to handle those? Also, thank you for the great library! |
You're very welcome, no problem.
Could you be a little more concrete + describe the kind of error you'd like to show the user? There's no hook to If you're looking for general connection status/change info, that's all published properly to the chsk channel. Does that make sense? |
I'm actually trying to do something similar as well here. In my case the user enters data on the client, then on blur I fire an event to the server. So in case where the connection is interrupted I need to pop up a modal to tell the user that their connection to the server is gone and data won't be persisted. I was trying to use (sente/start-chsk-router!
(:ch-recv @connection)
(event-msg-handler
{:message message-handler
:state handshake-handler
:handshake state-handler})
{:error-handler (fn [e event-msg] (println "error:" e))}) Sente will log the error:
What the recommended way to hook into that and provide an additional custom error handler for that event. |
Hi Dmitri, Is there a reason the standard chsk state change events don't cover your use case? If you watch your channel socket for |
Also, just to clarify: if you're sending a synchronous request to the server, the standard way of knowing if something went wrong is to check the callback value. The callback value will indicate if there was a broken connection, request timeout, etc. Does that help / make sense? |
Hi Peter, Thank you for your responses. A couple of concrete use cases that would be helpful to support:
I don't believe chsk-state has the granularity to support these yet, and am thinking that extending it to have an error state or exposing onerror could. Do you think either of these would be a good addition, or are there alternate approaches you're aware of? |
Hi Peter, Thanks for the quick response, that definitely helps. I think I just missed the state event for dropped connection. |
No problem. Reason you'd want the state event rather than trying to hook directly into WebSocket handlers is that the state events basically provides a consistent, dependable user-level API that can pave over underlying implementation issues like protocol differences, browser issues, etc. For example, you wouldn't want to have to handle onerror messages for each failed retry attempt - so the state channel just publishes the state deltas that would actually be useful at a user level. If that makes sense. |
You can use the
/chsk or auth request issues are something that you can + should usu. be handling at your auth request - again, unless I'm misunderstanding. For example: if you have an auth procedure that establishes a session through an Ajax auth request (as in the reference example) - then you can deal with auth problems directly when you receive the auth response? Does that make sense? Would suggest checking out the ref example if you haven't since that might help clarify. Cheers :-) |
For the UI bar, which :chsk/state event would i check? (In the case of a server down, the socket never connects, so there's no dropped connection event?) For the auth case, assume the client already successfully received a long-lived token (not session based), and the /chsk endpoint is simply checking that token to make sure it hasn't expired and returning 403 if so. I could easily be missing something, but don't believe these are covered currently. Appreciate your thoughts and thanks again! |
First successful connection, you'll see Please see the ref example project for more info on the
Not sure exactly what you mean by this exactly, would need a more concrete example. Generally speaking- if you want to do auth via an expiring token, you'd need+want to handle that at an API call level. I.e. at your event handlers check for your token state, etc. If the token's expired, respond to the request with a notification that the client can identify (something like You can wrap server or client side event handlers in middleware easily enough to get whatever behaviour you like. Trying to tie into the low-level WebSocket onerror handler here would be, I think, a bad idea. Actually, onerror wouldn't fire for the kinds of auth problems you're talking about. Please note that there's a summary of auth approaches at #118 (comment) Sorry if this was a bit terse, written in a hurry since I'm just handling some urgent stuff atm. |
Yeah this approach makes a lot of sense. The state events allow watching for changes in state and reacting to those. I definitely agree that it provides a good abstraction for this. |
I'm asking about the scenario in which the socket never connects in the first place.
Auth scenario:
I think the summary of auth approaches in #118 assume a web context with session-based auth, whereas this scenario is for a native context with token-based auth. Do these scenarios make sense? Thanks again and appreciate your time!! |
Alternative (recommended) scenario #1:
So the recommended approach is: start with a UI that informs the user that the app is disconnected/connecting. Iff the chsk state is open, remove that message. Auth scenario:
How is the server returning 403? That's not a standard part of the /chsk implementation. I recommend that you do application-level auth via an application-level auth req+resp. If your application-level auth token has expired, then your application-level API calls should fail with an appropriate application-level error code/message. Trying to tie application-level auth into the implementation-level socket might work in some cases, but is a pattern I'd like to discourage since it's encumbering w/o (as far as I can see so far) any compelling advantages. Can you articulate why you're not fond of the solution I suggested (event handling middleware)? Just a heads-up: unfortunately might not be able to continue this discussion for a few days while I have some urgent work I need to concentrate on, sorry. |
Hi Peter, A couple of thoughts:
I think "connecting/disconnected" is semantically different from "error" because in the former case, there's an expectation that the socket will soon connect, whereas in the latter it is known that it won't. It is good to be able to inform the user when there's an error with the service so that he doesn't keep trying and get frustrated with our apps.
I agree with you event-level auth is a valid approach, but if we can't handle 403s from the /chsk endpoint, then we have to leave the endpoint unsecured until after socket initialization (even session-based auth via ring middleware can return 403s), which I believe could open up a DOS attack vector. Authenticating the client during the initial websocket handshake is recommended by heroku for example: https://devcenter.heroku.com/articles/websocket-security#authentication-authorization
Hope these points make sense and please take your time responding. Appreciate the discussion! |
May I suggest using a timeout to notify the user after a few seconds "It's taking a long time to connect to the server"? Connecting to a webserver that is actually down (as in you have a route to it's IP, but nothing is answering) can take a very long time. |
Hi @theasp, while that would handle server timeouts specifically, I think it's cleaner for Sente to set its own timeouts and then simply expose errors via an onerror-fn or an :error chsk-state, which has the advantage of exposing all types of server errors (e.g. 503s) as soon as they're known. What do you think about that solution? |
Hey Daniel, I appreciate your thoughts and open-mindedness on this. I don't disagree with anything you've proposed, exactly - but do have in mind some tradeoffs that motivate the current design and that might be worth explaining. Just in the middle of a project launch atm, but promise I'll try get back to you on this when I have an opportunity. In the meantime, would suggest either following @theasp's suggestion, or perhaps running from your fork in the meantime until we get a chance to talk? Thanks again for the patience, cheers! :-) |
Quick update: haven't reached any firm conclusions on this yet. But till then, happy to keep the commit at f0785fd in as experimental so that I can get a sense of how folks end up using this. Thanks again for the PR + all the input on this. |
@drhops Hi Daniel, would like to come back to this for a moment since you mentioned that you've been satisfied with the new Had reason to just look into this a little more, and was surprised to see that the underlying So this has me curious: in what way is this event actually useful to you? How are you actually using it? All the info I'd consider useful for debugging seems to be available only in the onclose event. Am I missing something obvious? |
Will get back to you on this next week. |
…k/state` Specifically: a channel socket's state atom may now contain keys: :last-ws-error {:uuid _ :ev _ <...>} :last-ws-close {:uuid _ :ev _ <...>} So interested consumers can either watch the state atom for changes, or check the content of `[:chsk/state [<old-state-map> <new-state-map>]]` events. This approach should be fundamentally more flexible, and it has the added benefit of not needing to introduce any new `:chsk/_` event ids. Feedback welcome!
Gave this some more thought and have decided to drop the Instead, will now attach If you're interested in this info, you can either watch the standard state atom for changes - or handle the standard This new approach provides more info, provides an atomic snapshot of all info, is a lot more flexible, and doesn't need the introduction of a new Sente's own auto websocket->ajax fallback logic is now implemented entirely using this same (state snapshot) info. See https://github.com/ptaoussanis/sente/releases/tag/v1.9.0-beta3 for full release info and 9a7e172 for the relevant commit. Feel free to reopen if anyone still has any other feedback/requests. Cheers! |
Nice approach! |
If during websocket initialization a server is down or returns 403 due to invalid auth, we'll hit the
onerror
handler here:https://github.com/ptaoussanis/sente/blob/2db8e6e43dcb1b55583fd5a4ce6a661583b1878d/src/taoensso/sente.cljx#L820
One could override
onerror
like the following, but it's hacky and outside of the context of sente's other retry logic:Should there be a way to override that behavior (e.g. retry) more straightforwardly?
The text was updated successfully, but these errors were encountered: