370245 - Make the HTTP server asynchronous

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: WIP

I didn't want server asynchronicity initially because it made the implementation harder to understand, but it's getting to be a requirement for some needed functionality.

I have half the async done in the attachment.  It makes incoming data be processed asynchronously (no reading from a blocking stream, except for the request handler!), and it seems to work fairly well, judging from telnet sessions with specific request data and from running an Apache benchmark program on it.  What remains to be done is allowing request handlers to control some of the asynchronicity, which means (preferably at the handler's preference) eliminating the use of a storage stream for storing the data generated for every request.  This also requires adding some sort of API for handlers to determine how buffering works -- or it means that we must impose constraints like setting all headers and the status line prior to writing any data to the body of the response.  I don't really want to do this, but it might be an acceptable restriction.

I also think the architectural changes will make other things easier, like redirects, but I haven't given it quite as much thought as I should yet.  This patch also isn't anywhere near done yet, so it's going to go through a lot of change before it gets anywhere near the tree.

Somewhat amusingly, if you run the Apache benchmark program on this, the server socket itself fails (PR_Accept failure) and shuts down the server, but the server's code itself doesn't fail.  :-)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: More WIP

Comment 2

[Jeff Walden [:Waldo]]

Update on progress: things are going reasonably well, but I hit a snag that a request reading from the body pipe does so on the same thread which receives onDataAvailable calls via the stream pump -- hang.  biesi's suggested a fix which I haven't had time to implement yet; hopefully I'll get to it over the weekend.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Experimental 1.9-land thread-using (!) patch

Writing to the pipe and reading from it must happen on different threads, and I'm not sure there's an easy way to control that from non-trunk code.  This seems to work fine, except for POST support, except that I get a bunch of threadsafety assertions -- I need to ask some 1.9-era threads gurus about them, because I think I'm doing everything the right way.  (I hope I'm not, because if I am there's a serious problem with how new-world threads work.)

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch (nothing new, but makes future improvements easier)

Without threads or an async request-handling API, we can't reasonably handle request bodies without blocking the entire server reading a slow-to-arrive body.  I still need to think more about the right async API, and XPConnect blocks handling each request in its own thread and punting the issue, so this just makes things async but doesn't make any other improvements.  However, code quality's a bit better in terms of splitting up reading the data, processing it, and handling the request metadata after it's been constructed.

This patch also makes the server require 1.9; I don't get assertions if I change the target passed to asyncWait to null, but I do get wrapper usage errors that massively spam the console.  Forcing main thread for processing the entire request is the only way I found I could do this without assertions or errors printed to the console.  Someone who didn't care about either of these could probably change some of the thread stuff to null and omit other parts to make it work in 1.8 and maybe earlier, if he wanted.

I need to set up some server tests that feed raw data to the server soon, in precise chunks, to test that this all works fully correctly, but this is large enough I'd like to get it in first and follow up with that later.  I also don't know whether I have enough control over channels to do that (i.e. whether flush() would actually cause the data to be written to the socket or whether something in between there and the socket, maybe the OS, might do any buffering), and figuring out a way to do that that's resilient to server response changes (i.e. not just byte-by-byte equality with expected response) is hard.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 275376 [details] [diff] [review]
Patch (nothing new, but makes future improvements easier)

README:

If you want a timeout, use:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsISocketTransport.idl#102

There is no timeout by default.

httpd.js:

+    // Order is important below: we must decrement handler._pendingRequests
+    // BEFORE calling this.stop(), if needed, because this.stop() returns only

So, you never call this.stop(). Do you mean destroy()?

What assertions did you get when using threads/which objects weren't threadsafe?

+    var handler = this.server._handler;
+    var connection = this;
+
+    handler.handleError(code, connection, metadata);

is there a particular reason for the local variables here?

And in other functions on this object too...


Perhaps LineData should accept lines that have only LF instead of CRLF too?

+      var copier = new StreamCopier(bodyStream, outStream,
+                                    gThreadManager.currentThread,
+                                    true, true, 8192);

why are you now passing a thread here?

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 275376 [details] [diff] [review]
Patch (nothing new, but makes future improvements easier)

(In reply to comment #5)
> If you want a timeout, use:
> http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsISocketTransport.idl#102

Yeah, just haven't gotten around to doing that yet...


> So, you never call this.stop(). Do you mean destroy()?

It was probably clearer earlier, but this.stop() is called if some error condition was hit and _doQuit is true.  Before it was called in, I think, this method; now it's called in connection.destroy().  I tweaked the comment to make that clearer.


> What assertions did you get when using threads/which objects weren't
> threadsafe?

nsSupportsString was one of the objects (I think the only one, but I'm not sure).  The assertion was the "<foo> not threadsafe" one (when JS GC caused them to be released from a thread other than the one on which they were were created).  Note that this can happen even if you use those things correctly, i.e. only on a single thread at a time, if you get unlucky with when GC happens -- even if you don't use any threads yourself, but some other JS code uses a thread!  The warning I got was this one:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp&rev=1.151&mark=3205-3230#3195

It's not an assertion, but it generates a *ton* of console spew -- every time the object in question was touched.  While I could theoretically ignore it, the warning reads as though it could have been an assertion, and I don't want people to have to put up with that much spew anyway.

Oddly, when I tried to reproduce the warnings/asserts by changing some of the places in the patch that use a specific target, I couldn't reproduce them, but at the moment I don't have the time to investigate and figure out whether those results are actually correct or not.


> +    var handler = this.server._handler;
> +    var connection = this;
> +
> +    handler.handleError(code, connection, metadata);
> 
> is there a particular reason for the local variables here?

Remnants of the thread-using patch, mostly, since |this| isn't the outer |this| for the function to be run in the event posted to a thread; I removed the local variables.


> Perhaps LineData should accept lines that have only LF instead of CRLF too?

I don't see anything in RFC2616 about accepting that in the interests of compatibility or anything like that.


> +      var copier = new StreamCopier(bodyStream, outStream,
> +                                    gThreadManager.currentThread,
> +                                    true, true, 8192);
> 
> why are you now passing a thread here?

It was warning/assertion-avoiding fairy dust, I think; I retested with it reverted to null and don't get asserts/warnings, so I made that change.


bsmedberg's granted blanket approval for testing fixes for now, so I committed this to trunk.

Comment 7

[Jeff Walden [:Waldo]]

Fixed.

Comment 8

[Jeff Walden [:Waldo]]

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

Comment 9

[Jeff Walden [:Waldo]]

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