Skip to content

/jquery

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

native abort does not abort jqXHR in 2.1.x #2079

Closed
salomvary opened this issue Feb 10, 2015 · 9 comments

Comments

Assignees

@timmywil timmywil

Labels
Ajax
Projects
None yet
Milestone
  1.12.0/2.2.0
5 participants
@salomvary @dmethvin @timmywil @mgol @markelog
@salomvary
Copy link

commented Feb 10, 2015

If the native XMLHttpRequest is aborted (eg. calling abort(), suspending the computer or unloading the page) jqXHR does not fire any callbacks. Expected: fire error and complete callbacks.

Test code to reproduce:

var wrappedXhr;
$.ajax({
    xhr: function() {
      wrappedXhr = jQuery.ajaxSettings.xhr();  
      return wrappedXhr;
    },
    url: '/echo/json/?delay=100',
    complete: console.log.bind(console, 'jQuery complete'),
    error: console.log.bind(console, 'jQuery error'),
    success: console.log.bind(console, 'jQuery success')
})
.progress(console.log.bind(console, 'jQuery progress'))
.fail(console.log.bind(console, 'jQuery fail'))
.always(console.log.bind(console, 'jQuery always'));
wrappedXhr.abort();

Test code on jsfiddle.

Note: the bug is not present in 1.11.x and 3.0 (master) versions.
@dmethvin

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 10, 2015

Since 1.11.x polls it would seem to be okay, as you say. Not really sure why 3.0/master is okay though, seems like we would need an onabort hander to be notified of this. I'm not seeing any notifications if I change the version to master.

@markelog markelog added the Ajax label Feb 11, 2015

@timmywil timmywil added the Needs review label May 5, 2015

@timmywil

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 5, 2015

Should we add an onabort handler in master?
@dmethvin

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 6, 2015

Yes, it looks like we need to add one so that compat and master will have the same behavior.

@timmywil timmywil removed the Needs review label May 6, 2015

@timmywil timmywil added this to the 3.0.0 milestone May 6, 2015

@timmywil timmywil added the Blocker label May 6, 2015

@timmywil timmywil self-assigned this May 18, 2015

@timmywil timmywil removed the Blocker label Jul 6, 2015

@timmywil

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 6, 2015

So, after digging into the issue, this isn't the blocker I thought it was. While we don't try to hide the native XHR, we don't encourage direct manipulation either. If you use jQuery's abort method instead, everything works fine: http://jsfiddle.net/timmywil/agg3ovxz/9/.

@dmethvin Do you think this is still worth a fix? One minor speedbump is that the status text for the OP's case in compat is "error" when it should be "abort", so fixing this requires changes on both branches and I'm not sure it's something we necessarily support.
@salomvary

This comment has been minimized.

Sign in to view
Copy link
Author

commented Jul 6, 2015

@timmywil using jQuery's abort method is not an option when the browser aborts the request for some reason (computer suspend for example).
@dmethvin

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 6, 2015

So just to refresh myself without having to re-investigate, does this affect our current 3.0 master or 3.0 compat? The initial comment said it did not.
@mgol

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 6, 2015

@dmethvin It affects compat in a way that no handler fires at all. It affects master in a way that the error handler is fired instead of the abort one.
@timmywil

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 6, 2015

Other way around.
@mgol

This comment has been minimized.

Sign in to view
Copy link
Member

commented Jul 6, 2015

Ah, I confused this with #2413.

timmywil added a commit to timmywil/jquery that referenced this issue Nov 2, 2015

@timmywil
Ajax: trigger error callback on native abort 
- I don't see a way to control statusText on a native abort.
  So, I left it as an error callback.

Fixes jquerygh-2079
f751a48

@timmywil timmywil added the Has Pull Request label Nov 2, 2015

@timmywil timmywil referenced this issue Nov 2, 2015

Closed

Ajax: trigger error callback on native abort #2684

timmywil added a commit to timmywil/jquery that referenced this issue Nov 2, 2015

@timmywil
Ajax: trigger error callback on native abort 
- I don't see a way to control statusText on a native abort.
  So, I left it as an error callback.

Fixes jquerygh-2079
b7ab67d

timmywil added a commit to timmywil/jquery that referenced this issue Nov 2, 2015

@timmywil
Ajax: trigger error callback on native abort 
- I don't see a way to control statusText on a native abort.
  So, I left it as an error callback.

Fixes jquerygh-2079
892b57b

timmywil added a commit to timmywil/jquery that referenced this issue Nov 2, 2015

@timmywil
Ajax: trigger error callback on native abort 
- IE9 does not have onabort. Do not support it there.

Fixes jquerygh-2079
cc96838

timmywil added a commit to timmywil/jquery that referenced this issue Nov 2, 2015

@timmywil
Ajax: trigger error callback on native abort 
- IE9 does not have onabort. Use onreadystatechange instead.

Fixes jquerygh-2079
c4edc19

timmywil added a commit to timmywil/jquery that referenced this issue Nov 2, 2015

@timmywil
Ajax: trigger error callback on native abort 
- IE9 does not have onabort. Use onreadystatechange instead.

Fixes jquerygh-2079
1db1447

@timmywil timmywil closed this in 76e9a95 Nov 3, 2015

@dmethvin dmethvin removed the Has Pull Request label Jan 7, 2016

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.