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

New throwing behavior of ajax-read #462

Open
danielsz opened this issue Jan 25, 2025 · 3 comments
Open

New throwing behavior of ajax-read #462

danielsz opened this issue Jan 25, 2025 · 3 comments

Comments

@danielsz
Copy link
Collaborator

danielsz commented Jan 25, 2025

Sente will throw an error when an ajax timeout occurs. This behavior was introduced recently, with 9da662c.

This is a reasonable decision, however I would like to make a couple of comments and raise a couple of questions.

  1. Most of the ajax timeouts I am seeing result from stale connections due to prolonged inactivity, for example, myself coming back to the browser after my laptop resumed from sleep. This cause for the error I consider benign.
  2. Logs on a busy production server will show many of those uncaught errors over time.
  3. We should probably explain that this is going to happen in the README, and show how to catch the error and where. Examples should be given.
  4. The ex-data for now contains only the value of :timeout-msecs. It should be enhanced with data from the request such as its origin. Presumably available from the function argument sch but I am currently blanking as to what and how much information can be recalled from that.

Another perspective on this is the following:

  • Do not throw the error by default, but as an opt-in setting in Sente's constructor.
  • Motivation is quality of life for the developer.
  • The assumption here is that most readout errors are harmless and more importantly, unrecoverable.
  • Throwing the error is probably reserved for debugging.

That's it from me as for this issue. I'll be happy to hear what you all think.

@ptaoussanis
Copy link
Member

@danielsz Hi Daniel, thanks for the considered feedback! 🙏

Sente will throw an error when an ajax timeout occurs

Just to clarify- this isn't Sente behaviour, but behaviour of some of the community adapters.

Unfortunately there's a recurring challenge that tends to occur with community adapters. An adapter will get submitted and included, but then the underlying server changes at some point - or there's some other motivation to make a change.

Then the original author/s are no longer available, and it falls on the community or me to try and maintain adapters for servers that we might not use or be familiar with.

An alternative would be to remove these 3rd-party adapters, to encourage potential authors to setup and maintain their own repo if there's interest. But in practice that of course also involves tradeoffs, and would probably mean reduced adapter availability.

This is something I'll reconsider for a future v2 though.

Anyway, on the present issue:

  • It's super helpful and very much appreciated to get detailed experience reports like this from folks actually using a particular server + adapter - thank you for that!

  • Apologies if the current behaviour isn't ideal!

  • I'm very, very open to PRs or ideas for how these adapters can be improved. Unfortunately it looks like it hasn't been very clear how best to handle this particular topic (Ajax timeouts for the Jetty and Undertow adapters).

On your specific thoughts:

Most of the ajax timeouts I am seeing result from stale connections due to prolonged inactivity, for example, myself coming back to the browser after my laptop resumed from sleep. This cause for the error I consider benign.

That does sounds like something that probably shouldn't be throwing an exception 👍

I'm not sure off-hand how http-kit handles these cases, that could informative (or conversely could show that http-kit's doing something worse ^^).

We should probably explain that this is going to happen in the README, and show how to catch the error and where. Examples should be given.

This sounds good to me if no better alternative behaviour exists that'd altogether avoid the need for catching and concern about this.

I've been juggling a lot lately, and the added lack of familiarity with the involved servers means that I haven't had an opportunity to properly dig into understanding this topic myself yet.

My hope has been that someone more familiar with the relevant servers could propose either:

  • An improvement to the server adapter/s, or
  • A clear description of some improvement needed from Sente or its interfaces to enable an improvement to the server adapter/s

Do not throw the error by default, but as an opt-in setting in Sente's constructor.

Just to add context, the reason for the present behaviour (to explicitly throw) - was a recent discussion where it was suggested that the prior behaviour (to not explicitly throw) was effectively anyway leading to exceptions, just non-specific ones that were difficult to identify.

So the hope was that 9da662c would be at least no worse, but hopefully clearer.

But again, I'm not in a good position to advise on this without much time spent digging. I'm definitely keen on concrete counter suggestions!

What'd be a good alternative to throwing, that wouldn't end up just percolating to a non-specific exception?

@danielsz
Copy link
Collaborator Author

Thank you, Peter. If proceeding step by step sounds like a good strategy, I would like to start with the question as to what is Sente's expected/desired behavior in the case discussed above, ie. when the user reopens his laptop, which I suppose boils down to loses network connectivity and regains it.

@ptaoussanis
Copy link
Member

You're welcome Daniel, thanks for looking into this 👍

what is Sente's expected/desired behavior in the case discussed above, ie. when the user reopens his laptop, which I suppose boils down to loses network connectivity and regains it

That's a good question to start 👍 Unless I'm missing something obvious (very possible), I can't think of any specific reason why one would ever need or want to specifically handle connection-level timeouts on the server-side.

Sente's request API offers:

  1. Client->server request (with/out ack)
  2. Server->user broadcast

For (1) the client just needs to know if the request was successful or not.

For (2) we don't need any particular confirmations. And if we did, this'd currently need to be handled manually by sending an ack event from the user's client/s after they've received the broadcast.

Where exactly are these timeout exceptions being thrown? Presumably during (1) in Ajax mode, when something with the connection went bad?

If so, that seems to me like a failure that can just be logged at a trace level and otherwise silently ignored.

Unless I'm missing something, this would be neither an exceptional nor actionable occurrence.

From the client's perspective, the relevant Ajax request would just timeout - triggering the usual client-side failed-request flow. And if the client's connection has gone bad, the client will attempt to reconnect.

What do you think?

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

No branches or pull requests

2 participants