396226 - Need way to delay server response in mochitest

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

To test bug 383331, I need a way to delay the server response for a stylesheet request while at the same time allowing a subframe to load.  I'm not seeing an obvious way to do that sort of thing.

Comment 1

[Jeff Walden [:Waldo]]

No, there isn't, really.  It requires some sort of async request processing API (and CGI support!) wherein requests are handled until they signal in some manner that they're done processing the request, and which allows the server to process that request lazily.  Currently the server asynchronously processes the request headers as they're received (with the right code to do that without *too* many tweaks to the body of the request -- another bug), but there's nothing in the server to respond asynchronously.

I can bump up the priority of this at my end a bit, although I'm working around classes and MIT now.  I don't have any real idea exactly when I might realistically have this finished, to be honest.

(This also blocks testing bug 84582; I've had a comment from you sitting in my bugmail folder for months reminding me I need to get this done sometime.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, I just wanted to get this on file so that it wouldn't just have to be you who worries about getting it done... ;)

Comment 3

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

what I need for XHR testing is to send the headers immediately to client, but
not the body, or at least some way to keep the connection still open for awhile
after writing all the data to the stream.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: WIP, v0.1

This passes server tests, at least.  :-)  No tests for the new API methods, no documentation for anything, pretty sure I've got a lot of XXX comments to address, definitely failing a |make check| in netwerk/test, but the hard part, as far as supporting existing uses, is done (and I suspect most of async-processing uses that this patch enables is close to done, too).

This patch mostly preserves backwards compatibility except in two areas.

1. Once you write to the body stream in a request handler, setting headers
   and modifying the status line is verboten.  We'll attempt to throw a
   specific error if you try to do this, but the intermediate pipe we're using
   to intercept this isn't guaranteed to catch this immediately after data is
   written.
2. We no longer send a Content-Length header with every response.  Such tests
   can just check the length of the response data, and this paves the way to
   handling incoming requests with really really really large bodies (say, for
   example, testing an HTTP request for a >4GB file, which is impossible now
   because the entire response is buffered before being sent).

This patch also blithely removes server-level PUT/DELETE support and short-circuits the existing test for that support.  There's an existing bug on doing that (bug 468443) that's just waiting for existing Mochitests to stop using it; I'll have to push on getting that bug fixed (I think it's waiting to be pushed now) before a final version of this could land.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: WIP, v0.2

This should be fully backwards compatible.  There really was too much whack-a-mole of fixing existing tests to deal with the problems I mentioned in the last comment, and it wasn't really clear that we could get away with making all custom handlers not have Content-Length headers, anyway -- some tests need their output size to be sent to actually test what they were intended to test.  Async responses, however, will not get a Content-Length header; maybe if we start sending chunked responses we can provide that at some point (although not in a way that's useful most of the time, since trailing headers won't show up by onStartRequest), but for now this is good enough.

Still no real documentation, IDL's out of date, haven't tested async responses even manually (let alone in automated tests), there may not be enough control over data transmission with this due to the stream copier, &c., but we're getting there...

Comment 7

[Jeff Walden [:Waldo]]

I have recently (unwittingly) been reminded that the state-preservation APIs refer to metadata.path, which is valid during the request handler call but might not be afterward -- don't remember lifetime or what we do when we're done with it in the server, but need to be sure it's always valid even during an async response -- or, perhaps better, just get it in a local var and close over that.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: WIP, v0.3: o hai, I changed ur API

You have to admit, it's getting better; getting better all the time...

Documentation updates proceed, one test added so far for the most trivial changes, still lots more of the latter to do, as well as a holistic review to convince myself the processing of requests never goes astray (right now I'm convinced it works generally but haven't examined corner cases yet).

This patch isn't as big as it looks because I had to change an API.  The nsIHttpServer.stop() method claimed to not return until all requests were done being processed, but as the Mochitest server demonstrates, that's a lie: in that case the /server/shutdown response still had yet to be sent by the time stop() returned.  That's not good, particularly in an async world, because an async response that shut down the server and then waited to continue sending the response would appear to be stopped when it wasn't, and in such cases the code that created the server might try to shut down and end up spewing Components assertions.  Half this patch is just fixing existing users to continue execution (or test completion, more often) in a callback, and that constitutes 40-50K of the patch size.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: WIP, v0.4: with more testing and method documentation

Now's where the fun really starts: adding tests for asynchronous behavior, adding tests for failures during asynchronous behavior, and hitting all the corner cases in async behavior.  Still a long ways to go on that front before I can point random test authors at this and be confident that any problems they hit will be of their own making...

Comment 10

[Jeff Walden [:Waldo]]

Attachment: WIP, v0.5

This patch is ready as far as I can see it -- except, as far as I've tested it, for a niggling problem with reftest, wherein the call to stop the HTTP server used in reftests is somehow, inexplicably, specifying an argument which is wholly absent by the time the HTTP server internals receive the function call.  I almost suspect an XPConnect bug here, to be honest, given that I used the exact same pattern in multiple other places throughout the tests that use the server.  Regardless, this is an apparently little problem, but it's impossible to hack around it even temporarily, so even considering making that a followup fix is out of the question.

Comment 11

[Jeff Walden [:Waldo]]

The profile I'm using to run the tests has a stale xpti.dat in in somehow with the nsIHttpServer info circa March -- probably when I originally created that profile.  Dumping the numeric id for nsIHttpServer shows an old IID, and in that version of the interface stop() has null arity, hence the argument in the implementation assumes the usual extra-argument value |undefined|

I don't know why that file isn't being regenerated every time I run a new build (after making in layout/tools/reftest, netwerk/test/httpserver, and browser/app) -- this is a debug build, so that should be happening during the component registration step, right?  Or does it matter that the xpt file isn't in reftest.jar but rather is implicit in the build itself?

Comment 12

[Benjamin Smedberg]

We're supposed to blow away the profile xpti.dat whenever the buildid changes. I don't know why that's not working, unless perhaps NO_EM_RESTART suppresses that behavior.

Comment 13

[Jeff Walden [:Waldo]]

Complete false alarm on that -- I had an extension I wrote installed in that profile, and it happened to include httpd.js and an ancient XPT file for it which then promptly (and silently, I might add) overrode the one used in the build.  With that extension removed everything works fine.

Just down to a few intermittent failures left to debug before this should be ready to go...

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Complete patch

Whee, I think this is done!

The test changes, for the most part, can be ignored.  They're basically to account for the changes in how a server can be shut down -- there's now a callback (required to be non-null partly to help developers who might miss this change and wonder why an unpushed test has started failing), and it's called once all current connections are completed.

The meat of the patch, the part touching the server, is organized around a couple principles.  First, getting asynchronous response processing must be opt-in; there are too many existing tests that assume otherwise to ever have a chance of changing that (not to mention the sync model's conveniently simple for most use cases).  Second, non-async responses must be as similar to current responses as possible -- so you get fun things related to adding a Content-Length header to responses (necessary for how some of our caching code works), for example.

The basic control flow before this patch was as follows:

nsHttpServer
- receive a connection
RequestReader
- async read data from that connection (packaged as a Connection)
- async read all headers, then the specified body
- finally pass the Request representation to
ServerHandler
- handleRequest/handleError find the request handler to produce the Response
- call the handler, it updates the Response, return
- now write out the head of the Response, then the body
- do all that synchronously, no responses being created or requests being incrementally processed during that time
- once body copy completed, end the Response, close the connection, and done

The basic control flow after this patch is similar but quite different in the later, now-sync stages:

nsHttpServer
- receive a connection
RequestReader
- async read data from that connection (packaged as a Connection)
- async read all headers, then the specified body
- finally pass the Request representation to
ServerHandler
- handleRequest/handleError find the request handler to produce the Response
- call the handler
- ...write out response head and body...sometime...
- once body copy completed, end the Response, close the connection, and done

The first few steps are basically unchanged except for modifications to how currently-running requests are tracked and used to determine when full server shutdown is complete; it really gets interesting inside request handlers.  First, current request handlers should just work -- if you want any special behavior, you have to opt out of the response being sent as soon as your handler returns.  For that, you have to indicate that you're going to do things in your own time, which happens by calling processAsync() on the response; that disables response completion when the handler returns.  From here, beyond the usual response modifications, you can (must, really, but this patch introduces no timeout mechanism) say you're done creating the response.  After this the body copy and connection close can happen as before.

More details:

processAsync() sets a flag that's checked when the handler returns which determines whether the sync-response-copy and connection close happen or whether they wait on the handler's "futured" actions to complete.

The ability to write to network incrementally means partially-sent requests must never be internally restarted -- can't undo what you've done in that case.  This requires some fun to produce new responses when handlers throw exceptions (which affects how SJS execution errors, 404s, and custom error handlers work) and to terminate the connection abruptly if an error is discovered after the point of no return.

Header flushing combined with body data writing is tricky because the latter implies headers are frozen and can't be changed.  With that you have to make sure the head can't be mutated after the response is partially sent -- a useful sequence point for API clients.

Since many use cases for testing purposes really really want to push data at exact times, I've had to roll a stream copier that'll do that exactly when data is written to the Response's bodyOutputStream.  nsAsyncStreamCopier and its buffering just can't cut it, alas.

Connection counting is much safer now, because *connections*, not requests, are tracked, and from earlier in the connection too.  The previous setup was unsound with respect to requests that had been partially received at shutdown time -- not something you'd see in test harnesses, but certainly possible in general.

Anyway, I think that covers things at a high level.  Direct questions, comments, etc. here, and with a little effort we can start using this for testing that's been held up sometimes for years -- even on 191, I optimistically predict.  Bandwidth shaping, here we come!

Comment 15

[Jeff Walden [:Waldo]]

Attachment: With slight unbitrottings for recent changes in m-c

Comment 16

[Honza Bambas (:mayhemer)]

Jeff, if the review is really urgent, you will probably have to find someone else to do it. I will probably not get to this sooner then in 3-4 weeks. I have some job-unrelated issues to solve right now, sorry.

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Unbitrotted, with new APIs for object-valued state storage added

sicking points out to me that with the previous patch and API, the only way to do delayed response finishing in response to an incoming request designed to trigger it is to poll -- a bit ugh.  Here's a new API added to do that (with a test of precisely that situation) on the server object itself and in the SJS global environment.  The SJS API, due to limitations of wrappers and sandbox properties, has to use a user-provided callback to expose the saved object, but it works well enough even if it's not aesthetically pleasing.

  void setObjectState(keyString, valueObject);
  void getObjectState(keyString, function(valueObject) : void);

Not sure where to direct the review request, will ponder for a bit...

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Unbitrotted again

For lack of a better reviewer, let's try sayrer, can always bounce somewhere else if necessary, I guess...

Comment 19

[Robert Sayre]

Comment on attachment 372483 [details] [diff] [review]
Unbitrotted again

> catch (e) { /* stream closed, just don't send any body data */ }

Do something better here.

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Final patch

"Do something better" actually meant remove the try-catch entirely and make the available data be 0 if there was no stream and its size otherwise; I'm not sure why that wasn't there before.

There was one other comment on IRC regarding _hasOpenConnections complaining that the way it determines whether there are any connections is obtuse and that I should use __count__.  I'd rather avoid SpiderMonkey extensions where possible when the extension can't easily be hacked in so that other JS engines could run it (I allow const because it's easy to change if necessary).  Another suggestion was to use ES5's Object.keys() when we implement it, but that method's linear in the number of properties -- as far as I can tell only a for-in that returns true as the body and returns false out of it is both O(1) and part of present or future specifications.

I ran reftests against this and hit a hang loading a 600K reftest font, a deadlock as the server tried to write data to a buffer that was full because a pending event to read that data in the browser hadn't been handled.  That required sending file data asynchronously, which worked well enough once I determined that accessing bodyOutputStream before calling processAsync wouldn't call _startAsyncProcessor until finish(), so the hang would happen all over again.  A simple addition of the call if the streams had already been constructed fixed that, and everything works well now.  Details are in the interdiff, but they don't seem complex enough to want a last once-over.

I plan to land this sometime in the next few days when the tree's relatively uncongested -- would rather not have to deal with noise from anyone else's pushes.  I'll probably leave DEBUG = true for a cycle to make sure everything works, then I'll switch it back to false once it's clear everything's okay.

Comment 21

[Jeff Walden [:Waldo]]

Wheeee!

http://hg.mozilla.org/mozilla-central/rev/963eb4e81327
http://hg.mozilla.org/mozilla-central/rev/657c42f2cd2a

Need to fix bug 488611 to determine why Windows is stupid, but otherwise this is ready to use!  See the IDL for documentation on how everything works for now, and I'll update the relevant places on MDC in a bit.  Once I get this cleaned up a bit I'll also try to land it on 191 -- it's testing-only and all that, and it would be very useful for video tests in m-c and such, because otherwise we'll be making branch changes we can't test in the same way as the corresponding m-c changes.

Comment 22

[Jeff Walden [:Waldo]]

https://developer.mozilla.org/En/Httpd.js/HTTP_server_for_unit_tests is now updated with a very brief sketch of how processAsync() works.  Landing on 191 is still pending on b4 being released.

Comment 23

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07087b6e5fa7

I updated the doc mentioned in comment 22 to reflect that this is available everywhere but 1.9.0, as well, so this is finished.

Comment 24

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/07087b6e5fa7

Is there a reason why reftest.js (at least) 1.9.1 code is different?
(I noticed when i tried to apply bug 468024 patch.)

Comment 25

[Serge Gautherie (:sgautherie)]

Jeff, ping?

Comment 26

[Jeff Walden [:Waldo]]

Sorry, I was out the 13th through the 19th, still digging myself out of the bugmail pile.

It's different because bug 477164 and a handful of other changes landed on trunk but not branch, and there wasn't a compelling reason to port them at the same time.  Also, the branch port was large enough as it is, and I was loath to unnecessarily bloat it with tangential changes.

Comment 27

[Jeff Walden [:Waldo]]

Somehow I missed documenting the new state APIs (and somehow the old ones had been neglected too!) when I wrote up docs on this; fixed that now in the same location:

https://developer.mozilla.org/En/Httpd.js/HTTP_server_for_unit_tests