Fix HTTP:411 error on non-chunking web servers occuring during POST or PUT operations. #994

Closed
wants to merge 1 commit into
from

Projects

None yet

9 participants

bmelton commented Oct 17, 2012

Problem description (old) is here: https://groups.google.com/forum/?fromgroups=#!topic/jquery-en/IoKJgc8jvcQ

Issue is that Nginx (without Chunkin support enabled, as found on Dotcloud) rejects PUT and POST delete requests as they don't have any message body. 

Many work around this in code by settings a data: {} attribute in params.  

My fix simply swaps {} for null as they are effectively the same and this will support more servers. 

I've checked against a few different servers, Apache & Nginx with and without chunking enabled, and this hasn't caused any problems that I've seen. 

@bmelton bmelton Fixes HTTP:411 errors on DELETE to non-chunk srvrs
Problem description (old) is here: https://groups.google.com/forum/?fromgroups=#!topic/jquery-en/IoKJgc8jvcQ

Issue is that Nginx (without Chunkin support enabled, as found on Dotcloud) rejects PUT and POST delete requests as they don't have any message body. 

Many work around this in code by settings a data: {} attribute in params.  

My fix simply swaps {} for null as they are effectively the same and this will support more servers. 

I've checked against a few different servers, Apache & Nginx with and without chunking enabled, and this hasn't caused any problems that I've seen. 
3fabac2
bmelton commented Oct 17, 2012

Sorry, my pull request was unclear.

The standard workaround for this fix is to do something like this in code:

$.ajax({
url: url,
data: {}
blah: blah
});

My fix corrects it at the xhr level, instead of sending 'null' when no paramaters are present, it just submits an empty data object of '{}'

Contributor
staabm commented Oct 17, 2012

any chance to get a unit test for this?

bmelton commented Oct 17, 2012

Sure thing. I'll throw one up as soon as I figure out how to use QUnit.

Member

Have you checked in the network tab of your favorite browser that you're not actually sending something like [Object object] on the wire? xhr.send's body argument is only supposed to deal with strings and null.

I'm also unsure if this won't fail for other backends.

bmelton commented Oct 17, 2012

It is setting the payload to [object Object], but I'm not sure how that should cause an issue if the payload were otherwise expected to be null, which should be true as this only gets executed in place of 'null'.

Member

Some backends do not look kindly on requests that have a body while they shouldn't. Your approach will create an non-empty body for every request that shouldn't have one.

bmelton commented Oct 17, 2012

Not to put you out, but do you know of any example backends I could test it against?

If I can get it to fail I can perhaps correct and resubmit.

Edit: Thanks for the suggestion -- I knew this would be a concern, I just wasn't able to make it fail in any of the environments I have access to.

Member

Well, there are actually several problems with the approach:

  • you assume that it is not OK to send an empty body. It's perfectly fine to issue a POST with an empty body
  • the backend script can rely on an empty body being an actual valid input (as opposed to an unexpected [Object object] string that's guaranteed to fail with any backend expecting a URL encoded param list as a body, that is 90% of them).
  • there are backends that do not like GET requests with a non-empty body. Can't remember off the top of my hat, but I definitely was pinged about this in the past.

Point is: why not just use an empty string? That way, you ensure the Content-Length header is filled (with 0) which is the actual issue with a 411 (the request not providing a Content-Length header). It's also what happens when you set the ajax data option to {} because it's serialized internally before getting passed to the xhr transport. Hell, it's probably possible to just set the header in that case and keep the body as null in the call to send. Why take the risk of sending a non-structured body that's pretty guaranteed to make the backend choke?

bmelton commented Oct 17, 2012

Thanks for the insight. I've just tested locally and can confirm that sending an empty string resolves the 411 errors I was experiencing.

So now, the obligatory dumb questions (as this is the first patch I've made) -- should I revoke this pull request, make the change and resubmit? Alternately, I realized after the fact that I made the pull request on the master branch, where should that go?

@jaubourg jaubourg closed this Oct 18, 2012
bmelton commented Oct 18, 2012

Awesome. Thanks for all the detail (I was expecting you to point me at TFM). I'll try testing the emptiness of the params and setting the Content-Length and see what kind of results I get.

Member

@bmelton, I accidently deleted the post I had made, would you be so kind as to email it to me so that I can repost it? j@ubourg.net is the mail address ;) Thanks

Member

Here is the deleted message for great justice(tm) and thanks to @bmelton. Some editing and reformatting happened.


Ah! Processes, processes! :)

So:

  • The first thing you should do is create an issue on the bug tracker. The issue should ideally provide a minimal test-case. Since this is a backend issue, you probably can't use a jsfiddle so you're better off with a page hosted on your server. Preferably, the js should be directly in the page itself.
  • Then, once the team assessed the bug and determined it actually exists and that it should be fixed, it's time to code.
    You create a new branch in your own clone of jQuery preferably named after the id of the bug.
  • The first thing you wanna do is create a unit test that fails against jQuery (in your case, you must have qunit running on Nginx for that though). Only then can you try and fix the issue.
  • Finally, the test passes and everybody rejoices! \o/ (that means you and your girlfriend who's unsure if you still exist or not).
  • You submit your pull request.
  • @rwldrn jumps on every extraneous or missing space.
  • @gibson042 tells you to invert all your variable declarations so as to make gzip (the deity he not-so-secretly venerates) happy.
  • @jaubourg discusses the merits (a little) and flaws (a lot) of your approach.
  • @scottgonzalez comes in and asks why in Hell we'd want to fix this issue.
  • @Krinkle explains some guy he knows uses Nginx to host the website of his pro-jQuery coffee-shop.
  • @dmethvin finally declares it's a good enough reason to fix the issue and goes walk his dog.
  • Adjustements are made (by you) and your girlfriend files a Missing Person Report.
  • The whole thing gets merged 18 months later after @mikesherov adds a comment asking for someone to "merge this damnit!", finally does it himself and accidently deletes 18 months of sizzle refactoring in the process.

As for the problem at hand, I'd be very interested to know if pre-setting the Content-Length to 0 in the headers map (main ajax file) wouldn't suffice. Wouldn't the browser override the value if we provide a body to send?

Member

Merge this damnit! ©

Member

Wow... that's disturbingly accurate...

Owner
gnarf commented Oct 19, 2012
  • @gnarf37 cries because @jaubourg forgot him

😺

Member

:P There's just that much silliness you can cram in a github comment ;)

Owner

Just like @jaubourg to oversimplify things! 😸 He forgets that if it's a new author we need to add them to AUTHORS.txt but half the time their github commit name/email is bogus so we need to get them to change it. Then we also have to ask them to 📝 the CLA.

Member

@dmethvin I thought this was already scary enough :P

Owner

@mikesherov stop reverting sizzle!

Member

@timmywil maybe if @jaubourg knew how to write a grunt plugin, I would be able to stop.

Member
ajpiano commented Oct 20, 2012

i lol'd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment