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

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

Comments

Projects
None yet
5 participants
@salomvary

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.

Show comment
Hide comment
@dmethvin

dmethvin Feb 10, 2015

Member

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.

Member

dmethvin 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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil May 5, 2015

Member

Should we add an onabort handler in master?

Member

timmywil commented May 5, 2015

Should we add an onabort handler in master?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 6, 2015

Member

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

Member

dmethvin 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.

Show comment
Hide comment
@timmywil

timmywil Jul 6, 2015

Member

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.

Member

timmywil 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.

Show comment
Hide comment
@salomvary

salomvary 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).

@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.

Show comment
Hide comment
@dmethvin

dmethvin Jul 6, 2015

Member

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.

Member

dmethvin 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.

Show comment
Hide comment
@mgol

mgol Jul 6, 2015

Member

@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.

Member

mgol 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.

Show comment
Hide comment
@timmywil

timmywil Jul 6, 2015

Member

Other way around.

Member

timmywil commented Jul 6, 2015

Other way around.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Jul 6, 2015

Member

Ah, I confused this with #2413.

Member

mgol commented Jul 6, 2015

Ah, I confused this with #2413.

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

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 gh-2079

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

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 gh-2079

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

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 gh-2079

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

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

Fixes gh-2079

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

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

Fixes gh-2079

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

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

Fixes gh-2079

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

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

@cssmagic cssmagic referenced this issue in cssmagic/ChangeLog May 18, 2016

Open

jQuery #5

@jquery jquery 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.