366371 - (js-post) Add POST support to the JS HTTP server

Status: RESOLVED DUPLICATE of bug 443610

(Testing :: General, defect)

Description

[Jeff Walden [:Waldo]]

Bug 354980 wants this for testing Airbag crashdata-sending functionality.  (Amazingly, POST support isn't a MUST in HTTP/1.1 -- only GET and HEAD are.  I wonder if there are any HTTP/1.1-compliant servers which don't implement POST.)

Comment 1

[(not currently active) Ted Mielczarek]

I don't think the crashreporter upload uses chunked transfer encoding, but I'm not really sure.  It does do multipart/form-data.  The Win32 client (the only one in Mozilla CVS right now) just uses the wininet API.  You can test the crashreporter client pretty easily, just grab a trunk zip build and edit crashreporter.ini to point change the upload URL, and then run crashreporter.exe <some file> and it will try to POST it to that URL.

Thanks!

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Some half-working POST support

With this patch, POST seems to work about half the time on exactly one manual testcase (/trace as the action of <http://web.mit.edu/jwalden/www/form.html>).  (The other half of the time it gets stuck in a blocking wait in readByteArray.)

The problem (maybe others too) I'm fighting is that the stream converter that lets me work with line-at-a-time data buffers data.  The current setting of 1024 will cause a failure every time (because the stream is blocking and we'd try to read when the cursor was at the end).  Setting it to 2 seems to work a bit better, but it still seems to fail every other time.  I may end up writing a wrapper/buffer with .unget for the actual input stream to deal with this, unfortunately.

Another option I'm pondering is attacking some of the C++ implementations and APIs to make them actually do what I want, but that breaks the server on non-trunk code (unless I just implement the wrapper anyway as fallback), which I'm hoping to avoid.  It's also considerably more pain and testing of other things to make sure they don't break, which I'd ideally like to avoid if the code already "works".

A second issue I need to face is that the body stream I expose can't be the ultra-thin wrapper it is now, because the underlying stream is blocking.  I need an intermediary stream which handles overreads by throwing so that poorly written request handlers can't hang the server (or something of that nature).

The patch also contains non-working hacked-off support for Transfer-Encoding: chunked messages.  I'll likely remove that from the final patch and make it a separate bug.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Implement POST support for binary/non-binary message bodies

Some notes:

- the input stream now must be closed when the connection is closed so that the
  message body is available for reading by request handlers; since the number of
  things to pass along is getting a bit big, maybe I should switch to some sort
  of container instead of a bunch of method arguments, sometime?
- if you can think of a better way to expose binary data, I'm all ears;
  nsIBinaryInputStream is a ridiculous amount of work (and is underspecified to
  boot), but there's no similar functionality elsewhere
- I'm shooting perf in the foot with LineStream, but I don't see a way around it
  given the amount of buffering the previous component did (which didn't even
  work all the time anyway); suggestions for obviating this (while still being
  guaranteed to avoid over-reading the stream and blocking) are welcome

For what it's worth, I doubt this is sufficient testing; I ran into numerous blocking issues when writing the testcase, and I honestly don't know what made the differences between the hanging versions and the non-hanging ones, for the most part.  (I think most of the problems were extrinsic to the server, tho, and were just things I happened to poke the wrong way, mostly.)  I expect any issues to be resolved as POST support sees wider use than just server tests.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Cut down a little on String.fromCharCode function calls

So, calling String.fromCharCode for every character in each line of headers is slow.  (For that matter, calling functions in SpiderMonkey is slow.)  We can cut this down a lot if we only convert to string when an entire line is assembled, since String.fromCharCode accepts a variable number of code points.  (There's still the argument limit to worry about, but divide-and-conquer can solve that in a reasonably efficient manner.)  Making this change should speed up running tests a little.

Also, I changed bodyStream to bodyInputStream in the test (I'd made the change in the server earlier but didn't rerun tests after making the change), and I tweaked the README a little.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Unbitrotted

I toyed around today with using a stream pump and a pipe to implement fail-fast reads past the end of the body of a message (and thus allowing a non-crippled interface for the body stream), but I couldn't get it to work right.  I'll keep trying to make it work over the next several days, but if I haven't found a better way before this is reviewed, partial support is better than none.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 252719 [details] [diff] [review]
Unbitrotted

I need to think about this more (and possibly fix a few other bugs first).

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, what i need in order to test bug 397878 and other cross-site XHR bugs is the ability to do both POST and OPTIONS requests to the server and both inspect what the request looks like, and control what response to say.

We don't need to do all that in *this* bug of course.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

We really need this for XHR testing.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #8)
> We really need this for XHR testing.

XHR-on-workers too!

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

This is now sort of fixed Bug 454217

Comment 11

[Jeff Walden [:Waldo]]

Moving httpd.js bugs to the new Testing :: httpd.js component; filter out this bugmail by searching for "ICanHasHttpd.jsComponent".

Comment 12

[Jeff Walden [:Waldo]]

...and changing the QA contact as well.  Filter on the string "BugzillaQAContactHandlingIsStupid".

Comment 13

[Jeff Walden [:Waldo]]

Yeah, fixed by bug 443610 and bug 454217, as amended by bug 462864.