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

Way to know timeout errors vs other errors? #88

Closed
heff opened this issue Jul 21, 2015 · 9 comments
Closed

Way to know timeout errors vs other errors? #88

heff opened this issue Jul 21, 2015 · 9 comments

Comments

@heff
Copy link

heff commented Jul 21, 2015

It looks like all errors call the same error function but we need to know specifically when it's a timeout error to inform an adaptive video algorithm. Am I missing anything, or any suggestions on how it should be added?

Thanks.

@naugtur
Copy link
Owner

naugtur commented Jul 22, 2015

We have only one callback and only one error object. The point here is to mimic request/request from node, so I'll check how timeout is resolved there and that's the guideline.

Could you add more context here?

@dmlap
Copy link
Contributor

dmlap commented Jul 22, 2015

@naugtur thanks for the quick response! We have a project that provides streaming video support for video.js and a big part of that is downloading video chunks over XHR. We have timeouts specified for individual requests so that we have an opportunity to switch down to a lower quality level if the viewer's bandwidth has changed dramatically. We currently have an XHR shim that allows us to determine whether a request failed due to a server error or an expired timeout and have different handling for both cases. We'd love to get rid of our own XHR and use this project but we think separate timeout handling is an important behavior.

I appreciate your suggestion to use .ontimeout instead of options.timeout. Native XHR timeouts aren't implemented on Android 4.x devices however, and we will probably be supporting that platform for awhile.

@naugtur
Copy link
Owner

naugtur commented Jul 22, 2015

In that case, I honestly don't see any difficulty in distinguishing between types of errors you can get.
In xhr2 if you get a response from the server, it's considered a success, so error===null. You get the response object with statusCode, body etc.
error has value only if a clientside error happens - no connection, timeout, illegal cross-domain request etc. In certain cases the error object is an error passed by the browser to .onerror callback, so if you get a generic "unknown" error it means one of the two:

  • timeout happened
  • user will not be able to access your resources for now (no connection, broken browser etc.)

This being said, we should be able to implement giving a direct hint in the error that you're dealing with a timeout.

BTW. I just tested xhr in android4 basic browser recently and all tests passed. Something about the timeout feature is failing in android2.

@heff
Copy link
Author

heff commented Jul 23, 2015

Great, thanks @naugtur. I'll let @dmlap comment on if "unknown" could be good enough.

But otherwise it seems like the timeout hint could be useful either way. Happy to help if you have any hints on how you'd like to see that implemented.

@dmlap
Copy link
Contributor

dmlap commented Jul 23, 2015

Hmm... we also abort XHRs from our code if the viewer seeked and we no longer need the chunk we had been downloading, for instance. I believe that is also considered an error status for the request. Would we be able to distinguish that scenario from a timeout? I guess we could decorate the request object before aborting it ourselves but that seems a little inelegant.

@naugtur
Copy link
Owner

naugtur commented Jul 23, 2015

Aborting the XHRs yourself will have different results depending on browsers.

This discussion is getting diluted, we need actions.
I have an idea: you provide a PR with new file in tests that has test cases for each pair that you need to distinguish and don't know how (request timeout vs manual abort, etc) and I write the code that recognizes which is which - in asserts or in xhr itself if I don't figure out a nice way to do that.

Thanks to that we'll be able to check different browsers too. (I sometimes have access to weird browsers, including on android2 or kindle3g)

@dmlap
Copy link
Contributor

dmlap commented Jul 23, 2015

Sounds like a great idea. I'll give it a shot.

@mattdesl
Copy link

mattdesl commented Sep 1, 2015

It seems like a convention in node to use ETIMEDOUT as the error.code. Might be useful. 😄
https://www.npmjs.com/package/timed-out

@naugtur
Copy link
Owner

naugtur commented Sep 12, 2015

Take a look: #92
This should cover your usecases. I'll merge and release soon. It's going to require a bump in minor, so before I do, I want to make sure it's complete.

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

4 participants