Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat($httpBackend): add timeout support for JSONP requests #2503

Closed
wants to merge 1 commit into from

Conversation

dbinit
Copy link
Contributor

@dbinit dbinit commented Apr 24, 2013

Documentation implies that timeout works for all requests, though it only works with XHR. Change $httpBackend to set a timeout for JSONP requests which will immediately resolve the request when fired. Completed requests will cancel the timeout.

@petebacondarwin
Copy link
Contributor

Hi @dbinit - Here is the list of things we need to check off before it could be merged. Also I want to check with the core team why this was not included originally. There may be a reason...

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update (if suitable)
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
  • All changes requested in review have been implemented

@petebacondarwin
Copy link
Contributor

By the way, you should only have two blank lines in your commit message - one beneath the header and one above the footer.

@dbinit
Copy link
Contributor Author

dbinit commented Apr 25, 2013

Thanks @petebacondarwin,

I'm a Google contractor, so don't know if I need to sign the CLA or not... I did just in case (full name: David Bennett).

I've updated the commit message. Do I need to submit it to ci.angularjs.org, or do you handle that on merge?

@petebacondarwin
Copy link
Contributor

I can push to CI server but it is having a few timeout issues right now which is why I haven't ticked the box.

Documentation implies that timeout works for all requests, though it
only works with XHR. To implement:
- Change $httpBackend to set a timeout for JSONP requests which will
immediately resolve the request when fired.
- Cancel the timeout when requests are completed.
@dbinit
Copy link
Contributor Author

dbinit commented Apr 30, 2013

Any update on this?

@petebacondarwin
Copy link
Contributor

CI server buid is green. Landed at cda7b71. Thanks

@ThomasBurleson
Copy link

Question!
If the JSONP request times out, these fixes resolve the request... so the promise is resolved( -1 )?
But should the request not be REJECTED instead using a reject( -1 )?

@dbinit
Copy link
Contributor Author

dbinit commented May 8, 2013

@ThomasBurleson, $http checks the status code and rejects the promise if it isn't a success code. See https://github.com/angular/angular.js/blob/master/src/ng/http.js#L939

@ThomasBurleson
Copy link

@dbinit

I see that now... and with some application code changes it now works!

My 2nd-level reject handler was not being called and I thought the issue was the originating resolve...
The issue, however, was that my 1st-level reject handler was doing this:

function onInterceptFault( fault ){
    // No interception currently needed
    return fault.data || -1;
},

returning instead of throwing

function onInterceptFault( fault ){
    // No interception currently needed
    throw fault.data || -1;
},

Without the throw, my 2nd-level resolve() handler was being invoked. I keep forgetting this $q behavior.

Thanks again.

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

Successfully merging this pull request may close these issues.

3 participants