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

reject request on XHR timeout #2581

Closed
wants to merge 1 commit into from

Conversation

zacharyhamm
Copy link
Contributor

Hacks xhrMock to support promises in the handlers so that a timeout
event can be fired and appropriately reject the request promise.

Description

Adds a handler to XMLHttpRequests 'ontimeout' event so that request will reject client-side timeouts.

Motivation and Context

Fixes #2559

How Has This Been Tested?

Tested with a test in test-request.js and also manual testing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation change

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated docs/change-log.md

Hacks xhrMock to support promises in the handlers so that a timeout
event can be fired and appropriately reject the request promise.
Comment on lines +158 to +163
xhr.ontimeout = function (ev) {
if (aborted) return;
var error = new Error("Request timeout");
error.timeout = ev.target.timeout;
reject(error);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you verified the onreadystatechange callback isn't also executed? If it is, you'll need to set aborted to true to keep anything else from firing.

@@ -155,6 +155,13 @@ module.exports = function($window, Promise, oncompletion) {
}
}

xhr.ontimeout = function (ev) {
if (aborted) return;
var error = new Error("Request timeout");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be a little clearer on the error message

Suggested change
var error = new Error("Request timeout");
var error = new Error("Request timed timeout");

Comment on lines +21 to +23
return new Promise(function (resolve) {
resolve({status: 200, responseText: JSON.stringify({a: 1})})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this.

Comment on lines +699 to +701
return new Promise(function (resolve) {
resolve({status: 200, responseText: JSON.stringify({a: 1})})
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this.

@dead-claudia
Copy link
Member

@zacharyhamm Ping?

@zacharyhamm
Copy link
Contributor Author

zacharyhamm commented May 18, 2020 via email

@orbitbot
Copy link
Member

orbitbot commented Jul 5, 2020

@zacharyhamm Any chance you could have a look within the upcoming week or so? 🙂

@orbitbot orbitbot self-assigned this Aug 11, 2020
@kevinfiol
Copy link
Contributor

I replicated @zacharyhamm 's fix on flems.io here (make sure you enable throttling in your browser). I'm still not getting the proper reject after enabling throttling. Please let me know if I missed something in my example.

It seems the issue is that as @isiahmeadows suggests, the xhr.onreadystatechange event may execute as well. In this case, the reject() call on L149 of request.js executes before xhr.ontimeout gets a chance to run.

Assuming there was an error, and the xhr.status is still 0, it might make sense to do something like this so that xhr.ontimeout gets calls correctly:

if (success) resolve(response)
else {
    if (xhr.status === 0) {
        aborted = true
        return
    }
    try { message = ev.target.responseText }
    catch (e) { message = response }
    var error = new Error(message)
    error.code = ev.target.status
    error.response = response
    reject(error)
}

L142

And then:

xhr.ontimeout = function (ev) {
    var error = new Error("Request timed out");
    error.code = ev.target.status;
    reject(error);
}

L158

See flems of this

@kevinfiol kevinfiol mentioned this pull request Dec 16, 2020
11 tasks
@kevinfiol
Copy link
Contributor

Created a separate PR #2646

@orbitbot
Copy link
Member

Closed in favor of #2646 which has been merged.

@orbitbot orbitbot closed this Jan 30, 2021
@JAForbes JAForbes mentioned this pull request Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

Timeout error in m.request doesn't seem to be handled
4 participants