469228 - Support keep-alive connections in httpd.js

Status: NEW

(Testing :: General, task, P3)

Description

[Jeff Walden [:Waldo]]

Keep-alive, when both client and server support it, allows a single connection to be reused for multiple requests.  The client writes out a request, the server reads it and responds, and then the client can write another request to the same connection, the server can read it and send another response, &c.

Bug 466080 requires this, and it's good to do anyway, probably would speed up Mochitests somewhat too to not have to open one TCP connection per request.

(I initially misunderstood keep-alive to think it required server async response handling, but it doesn't, mostly just need to change the post-request-processed behavior, that's all.)

Comment 1

[Jeff Walden [:Waldo]]

Some handy docs on all this stuff, for reference:

http://www.io.com/~maus/HttpKeepAlive.html (high-level behavior description)
http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html (section on connections)

Comment 2

[Jeff Walden [:Waldo]]

Attachment: WIP

I can run at least some Mochitests with this, but I also get a nice little hang on close, and the HTTP server tests themselves are hitting the same thing.  It's not yet clear whether the keep-alive timeout pref is at fault here, but odds are no given that the changes here are somewhat complicated.

This doesn't provide pipelining support, but that shouldn't be too difficult to add -- I think.  The most obvious problem (may or may not be others) is that we copy the body of responses asynchronously, and we need to make sure we don't start sending a second request while still sending the first one.

Comment 3

[Jeff Walden [:Waldo]]

The WIP works only because of a reversed condition; turns out the way the request body is written to the network can't work because the stream-copier class closes the nsIOutputStream to which it writes, so I'm probably going to have to roll my own (although adding a flag to the built-in stream copier to control this would be good long-term).  That also runs into the problem of it being painful to write out header data before the body this way; I probably need to bite the bullet and say that the request body must only be accessed and written after all headers and such have been sent, so that I can just write it all into the same stream to copy to the network.  Still a ways to go here in any case...

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: part1: basic keep-alive support

Stealing from Jeff (as agreed in private email).

Since the httpd.js code is now very well written, it is extremely easy to allow keep of connections.  I reinstatiate RequestReader for each new request on a connection.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: part2: fix for the https schema

This is actually a continuation of fix from bug 468087 - we were not able to add just https hosts w/o having also http mirrors for them.  In that bug, we changed the ssltunnel code by introducing AdjustRequestURI function that updates the Request-URI to be a fully qualified URL.  This way httpd.js was able to figure out what scheme and (mainly) port the connection was created to.

With keep alive connections, subsequent requests are not updated and we are missing the port number information.

ssltunnel from simplicity reasons updates only the first request.  If we wanted to update all request we would have to very difficultly parse the incoming http requests and correctly find the head to update URIs, potentially on edges of buffers and with other problems.  Pretty overkill.

This patch simply uses the fact the first request has a fully qualified URI.  It caches the port number in member of the connection where from next RequestReaders grab it when don't know the port number to find the identity.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: part3: chunked encoding support

This supports chunked encoding when the response is processed asynchronously.  Chunked encoding is a precondition to keep the connection alive after an async response was finished and therefor also a precondition to have a pipeline-like processing of requests (i.e. to process another request when the current one is still being written).

To use it and keep the connection alive, just call processAsync with 'true' as an argument.  If that argument is omitted or 'false', then no Content-length nor Transfer-encoding header will be sent and connection will close after the response is finished.

We might turn chunked encoding on by default, but not sooner then the pipeline processing is done as it breaks some tests that finish()'es a response through object state using another request, that may schedule on the same connection, leading to a logical deadlock (can't call finish() an a request before finish() gets called on that request).

Please, mainly check I correctly write the EOF chunk.

Comment 7

[Honza Bambas (:mayhemer)]

All 3 patches will soon be updated and more patches added as well, though each one of them still quit simple.  I'm now trying to stabilize and fix tests that are obviously dependent on closing connections like xpcshell httpRawTests.

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: part1: v1 - basic keep-alive support

Resubmitting all patches now for full review, left split for easier review.

I have 5 code changing patches + 1 simple patch for fix of few httpd and network tests.  With few other separate test fixes (in the blocking bugs) the try server is nicely green:
http://tbpl.mozilla.org/?tree=Try&rev=9ccbf3c8710a

v1 is mostly identical to the draft + few small fixes.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: part2: v1 - support for https schema hosts

See comment 5.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: part3: v1 - chunked encoding support

See comment 6, expect that I have removed the argument (it was there just as an attempt to fix some sub-tests run issues).  Now, chunked encoding is always on.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: part4: v1 - pipelining

This adds support for something we might call pipelining.  If a response is processed asynchronously and another request is about to be read on the same connection then we don't block that incoming requests from being handled by the registered handler.

Responses are queued and only the first in the queue is capable of sending out data.

This is needed for some tests that keep a response blocked until an 'unblock' requests unblock it via set/getObjectState access.  And also is something we will need for true tests of the pipelining code.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: part5: v1 - keep-alive timeout

This adds keep-alive timeout for idle and never used connections.  Also plays with the Keep-alive header and timeout=N value.

Also adds API to nsIHttpServer to turn keep-alive completely off.  Some tests require it.

And also adds a fix for gc() call: I have no idea why, but w/o guarding the call with |if (typeof gc === "function")| reftest and debug crash test assert and hang during shutdown.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: part6: v1 - self and closely related tests fixes

Some tests need obvious fixes:
- check for "Connection: close" header changed to "keep-alive"
- raw-tests from their nature need to close the connection after the response is done
- one test was missing async close of the server

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: all v1 parts together - for reference

Comment 15

[Jeff Walden [:Waldo]]

Comment on attachment 549786 [details] [diff] [review]
part1: v1 - basic keep-alive support

Review of attachment 549786 [details] [diff] [review]:
-----------------------------------------------------------------

r- probably most substantively for exactly where the _closeConnection metadata is kept, but there's other stuff here too of somewhat less import.  Also, I haven't looked at the other patches here yet, but changes will need at least some basic tests, with the different combinations of keep-alive, close, HTTP/1.0, and HTTP/1.1 tested.  That's one thing I like about the current level of testing, that I can lean heavily on the existing tests sniffing out bugs for me, and if we're adding this complexity, we need to be able to keep doing that.  :-)

I'll keep looking at the subsequent patches on the assumption that there's not huge amounts of dependency among them, and that the basic concepts are reasonable even if the exact implementations might need changes.  Although it's getting late enough in the day that I may not get much beyond this patch, if at all, just to be clear.

::: netwerk/test/httpserver/httpd.js
@@ +473,5 @@
>    {
>      dumpn(">>> shutting down server on port " + socket.port);
> +
> +    // Close all idle connections now.
> +    for each (var connection in this._connections)

httpd.js uses no JS extensions except |const|, so |for (var id in this._connections) { var connection = this._connections[id]; ... }|.

@@ +1145,5 @@
>    this._closed = this._processed = false;
>  }
>  Connection.prototype =
>  {
> +  /** Create RequestReader instance and start reading from the socket */

This more describes the technical implementation details than describes what the method's supposed to do.  How about this instead:

@@ +1152,5 @@
> +    dumpn("*** read on connection " + this);
> +    this._processed = false;
> +    this.request = null;
> +
> +    this.server._connectionIdle(this);

This can close the connection, but the control flow for this is...less than clear.  If the connection's closed, we don't really want to go on to read in a request or do anything else, do we?  Maybe try-catches save us here, or something.  At best this is misleading but innocuous; at worst it'll cause bugs.

@@ +1160,5 @@
> +    // XXX add request timeout functionality here!
> +
> +    // Note: must use main thread here, or we might get a GC that will cause
> +    //       threadsafety assertions.  We really need to fix XPConnect so that
> +    //       you can actually do things in multi-threaded JS.  :-(

Since you're moving this code, this comment can go away; MT JS will never happen.

@@ +1227,5 @@
>      this.server._handler.handleError(code, this);
>    },
>  
> +  /**
> +   * Return true if the connection is currently not receiving nor sending

"not ... nor" isn't quite correct grammar.  You'd want "not ... or ...", or "is neither ... nor ...".

@@ +1231,5 @@
> +   * Return true if the connection is currently not receiving nor sending
> +   */
> +  isIdle: function()
> +  {
> +    return !this.request;

This can probably be narrowed to |this.request === null|.

@@ +3420,5 @@
> +   * Can be set to true whenever before end() method has been called.
> +   * If no connection header was present then default to "keep-alive" for
> +   * HTTP/1.1 and "close" for HTTP/1.0.
> +   */
> +  this._closeConnection = connection.request.hasHeader("Connection")

Connection closing doesn't seem to me to belong in Response code; it really belongs at the connection level.  Sure, you need to know what kind of Connection header to write out in the response, if one is needed.  But it seems to me the Response code should query the connection to determine how to do that, and shouldn't itself store that information.

@@ +3424,5 @@
> +  this._closeConnection = connection.request.hasHeader("Connection")
> +                        ? connection.request.getHeader("Connection")
> +                          .toLowerCase() == "close"
> +                        : connection.request._httpVersion.equals(
> +                          nsHttpVersion.HTTP_1_0);

Looking at RFC2616, it looks like the Connection header is a comma-separated token list, so you should split and check for "close".

Do you have a citation for the comparison being case-insensitive?  I'd prefer an exact, case-sensitive match if nothing says otherwise, on general be-strict-in-what-you-accept principles.

@@ +3608,5 @@
>      this._ensureAlive();
>  
>      dumpn("*** processing connection " + this._connection.number + " async");
>      this._processAsync = true;
> +    this._closeConnection = true;

(This looks to be in processAsync, if it's not then this might not apply.)

I don't immediately see why it should be impossible for async responses to work with keep-alive.  As long as they affirmatively indicate when the response is done -- which they do, as I recall -- it should be possible to make this work.  So at the very least file a followup bug on this, and put an XXX comment here.  (Unless all this gets resolved in the later patches in this bug -- I haven't looked at them at all.  :-) )

@@ +3659,5 @@
>          input.readByteArray(avail);
>      }
>  
>      this._powerSeized = true;
> +    this._closeConnection = true;

Since seizing power means you get to write out any sort of drek whatsoever, including drek that is completely malformed HTTP response data, this is very much appropriate.  I would add a comment clarifying that to the reader.

@@ +3807,5 @@
>    abort: function(e)
>    {
>      dumpn("*** abort(<" + e + ">)");
>  
> +    this._closeConnection = true;

Quite sensible.  I would add a comment here noting that because any of a variety of errors might have caused this, we can't know that the connection (either half) is in a consistent state to attempt to read another request, and that we give up entirely on this connection to avoid any further confusion.

@@ +3851,5 @@
>  
> +    if (this._closeConnection)
> +      this._connection.close();
> +    else
> +      this._connection.read(); /* restart reading this keep-alive connection */

These four lines should be in a method on Connection.prototype, I think -- a method called responseFinished() or something.  Again, whether the connection gets closed or the next request gets read should be left to Connection code, not to Response code.

@@ +3920,5 @@
> +    {
> +      // If "Connection: close" header had been manually set on the response,
> +      // then set our _closeConnection flag, otherwise leave it as is.
> +      this._closeConnection = this._closeConnection ||
> +        (headers.getHeader("Connection").toLowerCase() == "close");

This should propagate up to the Connection code here, of course.

@@ +3926,5 @@
> +    else
> +    {
> +      // There is no Connection header set on the response, set it by state
> +      // of the _closeConnection flag.
> +      var connectionHeaderValue = (this._closeConnection)

And this would query the Connection for whether the connection should be closed, then set the appropriate header.

Comment 16

[Honza Bambas (:mayhemer)]

Thanks for the review.

First and important thing: all part1 to part6 patches should be finally merged to a single one and landed as one or be landed in a single shot and in case of problems also backed out as one.  None of them alone is stable, tree will orange.  It just shows how things has been evolving.

(In reply to comment #15)
> but changes will need
> at least some basic tests, with the different combinations of keep-alive,
> close, HTTP/1.0, and HTTP/1.1 tested.  

I fully agree.  I will shortly start working on tests for this.

> on the assumption that there's
> not huge amounts of dependency among them, 

There are, I was trying to have incrementing snapshots how the work was developing between major stages.  So, some things change or move in following parts.  If you are not certain about pieces, then check the merged patch before commenting, but maybe I really should rather go with just a single patch for review.  Your comments show that it makes the review harder then simpler for you, IMO.

> 
> @@ +1145,5 @@
> >    this._closed = this._processed = false;
> >  }
> >  Connection.prototype =
> >  {
> > +  /** Create RequestReader instance and start reading from the socket */
> 
> This more describes the technical implementation details than describes what
> the method's supposed to do.  How about this instead:

?

"Wait for an incoming data on the connection expecting it to be a new full request" or something like this?

> 
> @@ +1152,5 @@
> > +    dumpn("*** read on connection " + this);
> > +    this._processed = false;
> > +    this.request = null;
> > +
> > +    this.server._connectionIdle(this);
> 
> This can close the connection, but the control flow for this is...less than
> clear.  If the connection's closed, we don't really want to go on to read in
> a request or do anything else, do we?  Maybe try-catches save us here, or
> something.  At best this is misleading but innocuous; at worst it'll cause
> bugs.

Yes, this will change in part4, mostly the way you have pointed out...

> @@ +3420,5 @@
> > +   * Can be set to true whenever before end() method has been called.
> > +   * If no connection header was present then default to "keep-alive" for
> > +   * HTTP/1.1 and "close" for HTTP/1.0.
> > +   */
> > +  this._closeConnection = connection.request.hasHeader("Connection")
> 
> Connection closing doesn't seem to me to belong in Response code; it really
> belongs at the connection level.  Sure, you need to know what kind of
> Connection header to write out in the response, if one is needed.  But it
> seems to me the Response code should query the connection to determine how
> to do that, and shouldn't itself store that information.

The logic around this changes in part 4 and is extended in part 5, BTW, but probably not as you suggest.

However, if I understand what you suggest: you want to keep this flag on the connection and when it goes to an idle state, close it?  That is, any handler can tell the connection to close when the connection is idle?  But how the connection recognize after which response it has to close when there are others scheduled?  That is what part 4 allows and what is fully valid situation we want to write tests for (pipelining).

So, I don't think I agree with moving this particular flag to the connection.

I agree that the decision logic and call to connection.close() have to be encapsulated in the connection.  But I still think the _closeConnection flag should be (also?) kept on Response and read by Connection from it.

In part 4 there will be introduced a method Connection.responseDone(response) that will be called at the moment the response is sent out (completed) and can be removed from the queue and other may go on.  I believe you want to move the Connection.close() call logic exactly there.  I would agree with then then.

> 
> @@ +3424,5 @@
> > +  this._closeConnection = connection.request.hasHeader("Connection")
> > +                        ? connection.request.getHeader("Connection")
> > +                          .toLowerCase() == "close"
> > +                        : connection.request._httpVersion.equals(
> > +                          nsHttpVersion.HTTP_1_0);
> 
> Looking at RFC2616, it looks like the Connection header is a comma-separated
> token list, so you should split and check for "close".
> 
> Do you have a citation for the comparison being case-insensitive?  I'd
> prefer an exact, case-sensitive match if nothing says otherwise, on general
> be-strict-in-what-you-accept principles.

Good point.  Will update it.

> 
> @@ +3608,5 @@
> >      this._ensureAlive();
> >  
> >      dumpn("*** processing connection " + this._connection.number + " async");
> >      this._processAsync = true;
> > +    this._closeConnection = true;
> 
> (This looks to be in processAsync, if it's not then this might not apply.)

It is :)  diff doesn't put the right context :(

> 
> I don't immediately see why it should be impossible for async responses to
> work with keep-alive.  

Because unpatched httpd.js doesn't send the Content-length header when the response is processed asynchronously.  Then the only way to finish the response is to close the connection.

We need chunked encoding to allow async writes w/o a large buffering (undesirable, right?).  Done in part 3.

> As long as they affirmatively indicate when the
> response is done -- which they do, as I recall -- it should be possible to
> make this work.  So at the very least file a followup bug on this, and put
> an XXX comment here.  (Unless all this gets resolved in the later patches in
> this bug -- I haven't looked at them at all.  :-) )

It gets :)

> 
> @@ +3659,5 @@
> >          input.readByteArray(avail);
> >      }
> >  
> >      this._powerSeized = true;
> > +    this._closeConnection = true;
> 
> Since seizing power means you get to write out any sort of drek whatsoever,
> including drek that is completely malformed HTTP response data, this is very
> much appropriate.  I would add a comment clarifying that to the reader.

OK.  I was also thinking of adding an argument to the finish() method if to close the connection or not (defaults to true).  It might be useful.  But rather do that in a followup when really needed.

> @@ +3851,5 @@
> >  
> > +    if (this._closeConnection)
> > +      this._connection.close();
> > +    else
> > +      this._connection.read(); /* restart reading this keep-alive connection */
> 
> These four lines should be in a method on Connection.prototype, I think -- a
> method called responseFinished() or something.  Again, whether the
> connection gets closed or the next request gets read should be left to
> Connection code, not to Response code.

Agree and this changes following parts, as said earlier.

> @@ +3920,5 @@
> > +    {
> > +      // If "Connection: close" header had been manually set on the response,
> > +      // then set our _closeConnection flag, otherwise leave it as is.
> > +      this._closeConnection = this._closeConnection ||
> > +        (headers.getHeader("Connection").toLowerCase() == "close");
> 
> This should propagate up to the Connection code here, of course.

I actually don't agree.  But please outline in more detail what you suggest.  

My idea was to allow a request say through Connection: close header to explicitly close the connection.

> @@ +3926,5 @@
> > +    else
> > +    {
> > +      // There is no Connection header set on the response, set it by state
> > +      // of the _closeConnection flag.
> > +      var connectionHeaderValue = (this._closeConnection)
> 
> And this would query the Connection for whether the connection should be
> closed, then set the appropriate header.

No.  Or maybe I don't understand what is your idea.  Is that you want to have the default value if to close the connection or not held on the Connection class (based up on the Connection request header and also the request HTTP version) that can only be drop to 'false' - as I suggest - by a response based on its state (response Connection header, way of processing, whatever).  Is that so?  If yes, then how the flag on the connection will be set?  By calling a Connection object method referring the request currently read on the connection by a reader?  Probably called by the reader?

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 549788 [details] [diff] [review]
part2: v1 - support for https schema hosts

Review of attachment 549788 [details] [diff] [review]:
-----------------------------------------------------------------

I was never particularly happy with this system of inferring a port number, and I'm still not particularly happy with it.  But I still can't think of something much better here.  :-\

::: netwerk/test/httpserver/httpd.js
@@ +1598,5 @@
>          // requested URI be absolute (and thus contain the necessary
>          // information), let's assume HTTP will prevail and use that.
> +        // _connection._incomingPort is defaulted to 80 (HTTP) but can be
> +        // overwritten by the first tunneled request with a full specified URL.
> +        // See also RequestReader._parseRequestLine method.

RequestReader.prototype._parseRequestLine, please.

@@ +1767,5 @@
>  
>        if (!serverIdentity.has(scheme, host, port) || fullPath.charAt(0) != "/")
>          throw HTTP_400;
> +
> +      // Remember the port number we have determined.  Subsequest requests

Subsequent

Comment 18

[Jeff Walden [:Waldo]]

Comment on attachment 549792 [details] [diff] [review]
part3: v1 - chunked encoding support

Review of attachment 549792 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little leery of the way the chunking is finished off with the empty chunk.  And I'd like to see the final wait, which is currently try-caught with all exceptions swallowed, better explained.  But this seems pretty decent otherwise.  Still going through the others, tho, so leaving this for now given that these concerns don't quite seem negligible to me...

::: netwerk/test/httpserver/httpd.js
@@ +3510,5 @@
>    this._processAsync = false;
>  
>    /**
> +   * Flag indicating use of the chunked encoding to send the asynchronously
> +   * generated content.  Set as an argument to processAsync method.

True if this response's asynchronously-generated content is being transferred using the chunked Transfer-Encoding.  Note that when the content of a chunked response is completely sent, this reverts to false to permit the final, empty chunk to be written.

@@ +3708,5 @@
> +
> +    // If we are using chunked encoding then ensure body streams are present
> +    // to send the EOF chunk, for what WriteThroughCopier is responsible.
> +    if (this._chunked)
> +      this.bodyOutputStream;

Hmm.  This is a bit of a kludge.  Probably we should isolate the create-an-output-stream logic from the bodyOutputStream getter and call it directly here.  For now this is probably good enough, tho.

@@ +4170,5 @@
>   *   context passed to observer when notified of start/stop
>   * @throws NS_ERROR_NULL_POINTER
>   *   if source, sink, or observer are null
>   */
> +function WriteThroughCopier(source, sink, observer, context, chunked)

|chunked| should be documented just above, in the method description.

WriteThroughCopier was originally intended to give specific write-through semantics in case of error.  I'm not entirely sure what I think about overloading it with this new behavior.  But it seems reasonable enough to run with for now.

@@ +4188,5 @@
>    /** Context for the observer watching this. */
>    this._context = context;
>  
> +  /** Forces use of chunked encoding on the data */
> +  this._chunked = chunked;

Since this gets called with incomplete arguments sometimes, this value is either boolean or undefined, depending.  Make this |Boolean(chunked)| instead to reduce the possible values this can have.

@@ +4301,5 @@
>          bytesConsumed = data.length;
> +
> +        if (this._chunked)
> +        {
> +          // Apply chunked encoding here

As in normalizeFieldValue, please copy the ABNF for Chunked-Body here, and note that it came from section 3.6.1.  It's been awhile since I looked at RFC 2616 much, so my natural first thought was, "Does this match the spec?"  Copying in the ABNF makes that fairly clear to the reader.

@@ +4584,5 @@
> +    if (this._chunked)
> +    {
> +      // Write the final EOF chunk
> +      dumpn("*** _doneReadingSource - write EOF chunk");
> +      this._chunked = false;

Hmm.  So I guess you're unsetting chunked here so that when we reenter onOutputStreamReady, we don't double-encode this.  And you couldn't write an empty string or something because that wouldn't pass the bytesRead > 0 check.  I can't help think that writing out the trailer should really be in some sort of copying-finished code of some sort, not redirecting back through the same copy-to-sink code used earlier.  But I guess that would run into the async mess of wondering what would happen if the sink blocked doing the write, or between each byte, or somewhat.

Async is hard.  :-\

@@ +4591,5 @@
> +      {
> +        this._waitToWriteData();
> +        return;
> +      }
> +      catch (e) {}

What error are you catching here?  If there's some specific error you're deliberately swallowing, a comment is in order.  If there's not, um, a comment is still probably in order.

Comment 19

[Kristian Rosenvold]

I have tried to backport this patch to the 3.x compatible branch of firefox and I can't seem to get proper keepalives working on ff3.x (this is for the selenium project). Is this because we use asynch response, or is there some other reason for this ?

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 549798 [details] [diff] [review]
part6: v1 - self and closely related tests fixes

Review of attachment 549798 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_bug561042.js
@@ +18,5 @@
>    },
>  
>    onStopRequest: function (request, ctx, status) {
>        do_check_eq(status, Components.results.NS_OK);
> +      server.stop(do_test_finished);

I'm a bit surprised this test even worked before...

Comment 21

[Jeff Walden [:Waldo]]

(In reply to Kristian Rosenvold from comment #19)
> I have tried to backport this patch to the 3.x compatible branch of firefox
> and I can't seem to get proper keepalives working on ff3.x (this is for the
> selenium project). Is this because we use asynch response, or is there some
> other reason for this ?

I don't know; it would take me awhile to wrap my head around everything to be absolutely certain.  Async responses might do it.  As I noted earlier, this all needs a pretty large mess of tests for me to be particularly confident in it.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 549797 [details] [diff] [review]
part5: v1 - keep-alive timeout

Review of attachment 549797 [details] [diff] [review]:
-----------------------------------------------------------------

I am kind of +ing this against my better judgment in the interests of not blocking you for a couple weeks while I'm gone.  Not your fault, of course -- just the happenstance of when I had my vacation planned.  And probably I deserve a little blame for not getting to this review sooner, to allow extra turnaround time on responding to reviews.  :-\

::: netwerk/test/httpserver/httpd.js
@@ +404,5 @@
> +   * Flag for keep-alive behavior, true for default behavior, false for
> +   * closing connections after the first response.
> +   * Set by nsIHttpServer.keepAlive attribute.
> +   */
> +  this._defaultKeepAlive = true;

This shouldn't be named with "default" in the name: it kind of makes me think that its value is a constant (notwithstanding context).  But "_keepAlive" also sounds verb-y, not like a boolean field.  How about "keepAliveEnabled" (name the attribute in IDL "keepAliveEnabled", name the property "_keepAliveEnabled") instead?

It's not necessary to call out what attribute gets/sets a property, generally -- particularly if you make the naming change I suggest.

@@ +407,5 @@
> +   */
> +  this._defaultKeepAlive = true;
> +
> +  /**
> +   * The number of seconds to keep idle persistent connections alive

I think you could collapse this to a single line /** ... */ comment without going over 80 characters, but I'm only eyeballing it.

@@ +409,5 @@
> +
> +  /**
> +   * The number of seconds to keep idle persistent connections alive
> +   */
> +  this._defaultKeepAliveTimeout = 120;

2 * 60 might be slightly nicer to read.

@@ +716,5 @@
> +    return this._defaultKeepAlive;
> +  },
> +  set keepAlive(doKeepAlive)
> +  {
> +    return this._defaultKeepAlive = doKeepAlive;

The value returned by a setter function is, for better or worse, ignored.  So this shouldn't return a value.

@@ +813,5 @@
>        this._notifyStopped();
>      // Bug 508125: Add a GC here else we'll use gigabytes of memory running
>      // mochitests. We can't rely on xpcshell doing an automated GC, as that
>      // would interfere with testing GC stuff...
> +    if (typeof gc === "function") gc();

if (...)
  gc();

@@ +822,5 @@
>     * may be closed when the server went down.
>     */
>    _connectionIdle: function(connection)
>    {
>      // If the server is down, close any now idle connections.

"now-idle", as the two words together form an adjective for "connections".

@@ +1200,5 @@
>    this._responseQueue = [];
> +
> +  /**
> +   * The current keep-alive timeout value.  Adjustable by Keep-alive: timeout
> +   * header in requests and responses.  Units: seconds.

"The current keep-alive timeout duration, in seconds.  Individual requests/responses can modify this value through the Keep-Alive header."

@@ +1205,5 @@
> +   */
> +  this._keepAliveTimeout = server._defaultKeepAliveTimeout;
> +
> +  /**
> +   * Start the initial timeout, if no data are read on this connection within

Period after timeout, because the first three words are a distinct sentence, as are the remaining words of this comment.

"Data" is the plural of "datum", to be sure.  But it is also a perfectly valid collective noun in its own right.  And it reads much better when used that way (at least here), too.  So make this "If no data is read from...".

@@ +1264,5 @@
>        server.stop(function() { /* not like we can do anything better */ });
>    },
>  
> +  /** Let the connection be persistent for the keep-alive time */
> +  persist: function()

I'm pretty sure we could do the cancel-and-reinit thing pretty nicely, but this is already written, and I'm running out of time to review a patch which did it better.  I'm still pretty sure that's possible, notwithstanding your comments here.  Maybe some other time.

It would be nice if you abstracted the cancel-and-null bit into a method, tho, to centralize that logic just a little bit more.

@@ +1339,5 @@
>        this.server._connectionIdle(this);
>    },
>  
>    /**
> +   * Return true if the connection is currently not receiving nor sending.

This probably doesn't matter given the comment further down, but for future reference: "not...or...", not "not...nor...".

@@ +1346,2 @@
>     */
>    isIdle: function()

The description should describe the semantics it implements.  It doesn't quite do this right now.  How about this instead:

"Returns true iff this connection is not closed and is idle.  A connection is idle when all requests received on it have triggered responses, and those responses have been completely sent."

@@ +1346,4 @@
>     */
>    isIdle: function()
>    {
> +    return !this._responseQueue.length && !this.request && !this._closed;

length === 0, still.

@@ +4202,3 @@
>        // There is no Connection header set on the response, set it by state
>        // of the _closeConnection flag.
>        var connectionHeaderValue = (this._closeConnection)

I don't remember which patch added this (or even if it was that very first patch that I reviewed, offhand), but the condition should not be parenthesized.

@@ +4206,5 @@
>                                  : "keep-alive";
>        headers.setHeader("Connection", connectionHeaderValue, false);
>      }
>  
> +    if (headers.hasHeader("Keep-alive"))

Keep-Alive is a more common capitalization, so use that.

@@ +4210,5 @@
> +    if (headers.hasHeader("Keep-alive"))
> +    {
> +      // Read the Keep-alive header and set the timeout according it.
> +      var keepAliveTimeout = headers.getHeader("Keep-alive")
> +                             .match(/timeout\s*=\s*(\d+)/);

/^...$/ to be more restrictive about the exact syntax we demand, please.  I don't see a whole lot of value in supporting the 0# bit that RFC 2068 mentions.

Speaking of which, you should mention in a comment that this header is defined by RFC 2068, and perhaps mention that it's no longer in RFC 2616, maybe with a reference to the explanation why.

@@ +4212,5 @@
> +      // Read the Keep-alive header and set the timeout according it.
> +      var keepAliveTimeout = headers.getHeader("Keep-alive")
> +                             .match(/timeout\s*=\s*(\d+)/);
> +      if (keepAliveTimeout)
> +        this._connection._keepAliveTimeout = parseInt(keepAliveTimeout[1]);

parseInt without an explicit radix is dangerous.  Always explicitly specify a radix (10 here) when using parseInt.

::: netwerk/test/httpserver/nsIHttpServer.idl
@@ +242,5 @@
> +   *        and announce it with Connection: close header.
> +   *
> +   * Default value is TRUE.
> +   */
> +  attribute PRBool keepAlive;

The description here is a bit unusual as far as formatting goes.  I suggest something like this:

Configures use of keep-alive connections, wherein the same network connection may be used to receive and respond to multiple requests.  If true, keep-alive connections will be used when possible.  If false, keep-alive connections will not be used, and responses will include a "Connection: close" header.  Defaults to true.

Ideally I'd prefer slightly more detail concerning "when possible", but I don't actually remember keep-alive semantics all that well right now, and given that I'm going to be gone fairly shortly and have many other things to do (foremost among them clear out my review queue to not block anyone else, ideally), I don't really have the time to spend to investigate.  :-\  Also, I'm not sure what gradation of detail I'd want to include here.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 549795 [details] [diff] [review]
part4: v1 - pipelining

Review of attachment 549795 [details] [diff] [review]:
-----------------------------------------------------------------

I am at best ambivalent about processing multiple incoming requests on the same connection simultaneously (if only writing them out sequentially).  But again, I don't really want to keep you held up for a couple weeks while I disappear, so I am somewhat warily +ing this.

::: netwerk/test/httpserver/httpd.js
@@ +1163,5 @@
> +   * The current responses queue.  Each new response is pushed to this queue
> +   * and shifted out when done.  This way we keep track of responses to process
> +   * and let process request(s) sooner the previous response(s) were sent out.
> +   */
> +  this._responseQueue = [];

This has me leery.  I guess it's sort of the client's problem to be careful about not having too many responses written ahead of the server, in general (because it doesn't know how many the server might have received, or what side effects might have been performed).  Still, I would probably feel slightly easier if incoming processing stopped when a full request was received and only resumed when the full response to it had been generated.

(Or I'm reading this all wrong, and I shouldn't really be trying to churn through this review as fast as I can before I go, anyway.  :-\ )

@@ +1223,5 @@
>     */
>    process: function(request)
>    {
> +    NS_ASSERT(!this._closed);
> +    NS_ASSERT(request == this.request);

===.  Really, never use ==.  The conflating it does and the potential side effects it might trigger make it a bad idea.

@@ +1229,5 @@
>      this.server._handler.handleResponse(this);
> +
> +    this.request = null;
> +    if (this.isIdle())
> +      this.server._connectionIdle(this);

This temporary association between connection and request feels like it's tying too much logic into the Connection object to me.  I originally introduced Connection as a way to encapsulate only a fairly small bit of the nuts and bolts and mechanics of the socket transport for a connection.  Right now it doesn't do much more than consolidate connection closing and hand off a request to be handled.  I'm not sure I like stuffing particular request tie-in into it.  :-\

@@ +1259,5 @@
>     * Return true if the connection is currently not receiving nor sending
>     */
>    isIdle: function()
>    {
> +    return !this._responseQueue.length && !this.request;

Compare === 0 and === null, please.

@@ +1274,5 @@
> +   */
> +  queueResponse: function(response)
> +  {
> +    dumpn("*** queueResponse on connection " + this);
> +    if (this._responseQueue.length)

> 0

@@ +1295,5 @@
> +    NS_ASSERT(this._responseQueue[0] == response,
> +              "Shifting response not first in the queue??");
> +    this._responseQueue.shift();
> +
> +    if (this._responseQueue.length)

> 0

@@ +2379,5 @@
> +    // Now start reading from the connection again regardless the current
> +    // response processing state.  The following request will be read
> +    // and a response will be processed but will be blocked from sending to
> +    // the connection until the current response finishes being completely
> +    // sent out.

This is tricky, generating responses to requests before previous responses have been sent.  I would rather not see this complexity without a compelling reason for it.  I'm not sure I see one now.  Why not just process a request, generate the response, then read the next request, then generate the next responses, and so on?

@@ +3621,5 @@
>    this._powerSeized = false;
> +
> +  /**
> +   * Gets set to true if this is not the first response pending on the current
> +   * connection.  This flag is used to block this response from writting when

"writing"

@@ +4059,5 @@
> +    }
> +
> +    if (this._writeBlocked)
> +    {
> +      dumpn("*** response is blocked from writting");

"writing"

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 549792 [details] [diff] [review]
part3: v1 - chunked encoding support

Review of attachment 549792 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure what your timeline is for turning around on responding to this.  If you have time, I'd kind of like to see post-change stuff here before I go.  If not, I guess I'm okay with this stuff going on in my absence.  :-\

Comment 25

[Honza Bambas (:mayhemer)]

Jeff, just let you know I'll soon update these patches and re-request reviews where necessary.  It is getting actual again to have this functionality.

Thanks so far for your feedback.

Comment 26

[Honza Bambas (:mayhemer)]

Patrick, do you think this is worth to be revived and finished?

Comment 27

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #26)
> Patrick, do you think this is worth to be revived and finished?

its obviously not the highest value thing we could be doing.. so on that basis, not really. But I think we would benefit from having it done..

Comment 28

[cedric.chandon_bugzilla]

I was just looking to create a new issue for it, i see it already exists for a long time.

Comment 29

[Honza Bambas (:mayhemer)]

I'll release this bug for now.

Comment 30

[Kershaw Chang [:kershaw]]

I've just finished rebasing those patches. Still have to polish them based on Jeff's comments.

First things first, Honza, would you like to review your own patches? If yes, do you prefer review them piece by piece or in one single huge patch? IMO, a single patch in one file seems easier to maintain and review.

Comment 31

[Kershaw Chang [:kershaw]]

(In reply to Kershaw Chang [:kershaw] from comment #30)
> I've just finished rebasing those patches. Still have to polish them based
> on Jeff's comments.
> 
> First things first, Honza, would you like to review your own patches? If
> yes, do you prefer review them piece by piece or in one single huge patch?
> IMO, a single patch in one file seems easier to maintain and review.

That's a silly question. I think Jeff should be the reviewer.

Comment 32

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #31)
> (In reply to Kershaw Chang [:kershaw] from comment #30)
> > I've just finished rebasing those patches. Still have to polish them based
> > on Jeff's comments.

Cool!

> > 
> > First things first, Honza, would you like to review your own patches? If
> > yes, do you prefer review them piece by piece or in one single huge patch?
> > IMO, a single patch in one file seems easier to maintain and review.
> 
> That's a silly question. I think Jeff should be the reviewer.

Yes, I think it should be him :)  The patches overlap, so they are not good for review separately.  Maybe just keep them for reference.  But for review, please provide a complete (folded) patch.  Thanks!

Comment 33

[Kershaw Chang [:kershaw]]

Attachment: All parts: keep-alive, chunked and pipelining support

Hi Jeff,

This patch is basically based on Honza's patch and also your review comments.
Could you take a look again?

Thanks.



Reply your review comments (include part1~part5) below for reference.

> Comment on attachment 549786 [details] [diff] [review]
> part1: v1 - basic keep-alive support
> 
> Review of attachment 549786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/test/httpserver/httpd.js
> @@ +473,5 @@
> >    {
> >      dumpn(">>> shutting down server on port " + socket.port);
> > +
> > +    // Close all idle connections now.
> > +    for each (var connection in this._connections)
> 
> httpd.js uses no JS extensions except |const|, so |for (var id in
> this._connections) { var connection = this._connections[id]; ... }|.
> 

Done.

> @@ +1145,5 @@
> >    this._closed = this._processed = false;
> >  }
> >  Connection.prototype =
> >  {
> > +  /** Create RequestReader instance and start reading from the socket */
> 
> This more describes the technical implementation details than describes what
> the method's supposed to do.  How about this instead:
> 

Done.

> @@ +1152,5 @@
> > +    dumpn("*** read on connection " + this);
> > +    this._processed = false;
> > +    this.request = null;
> > +
> > +    this.server._connectionIdle(this);
> 
> This can close the connection, but the control flow for this is...less than
> clear.  If the connection's closed, we don't really want to go on to read in
> a request or do anything else, do we?  Maybe try-catches save us here, or
> something.  At best this is misleading but innocuous; at worst it'll cause
> bugs.
> 

Please see Honza’s reply in comment#16.

> @@ +1160,5 @@
> > +    // XXX add request timeout functionality here!
> > +
> > +    // Note: must use main thread here, or we might get a GC that will cause
> > +    //       threadsafety assertions.  We really need to fix XPConnect so that
> > +    //       you can actually do things in multi-threaded JS.  :-(
> 
> Since you're moving this code, this comment can go away; MT JS will never
> happen.
> 

Done.

> @@ +1227,5 @@
> >      this.server._handler.handleError(code, this);
> >    },
> >  
> > +  /**
> > +   * Return true if the connection is currently not receiving nor sending
> 
> "not ... nor" isn't quite correct grammar.  You'd want "not ... or ...", or
> "is neither ... nor ...".
> 

Done.

> @@ +1231,5 @@
> > +   * Return true if the connection is currently not receiving nor sending
> > +   */
> > +  isIdle: function()
> > +  {
> > +    return !this.request;
> 
> This can probably be narrowed to |this.request === null|.
> 

Done.

> @@ +3420,5 @@
> > +   * Can be set to true whenever before end() method has been called.
> > +   * If no connection header was present then default to "keep-alive" for
> > +   * HTTP/1.1 and "close" for HTTP/1.0.
> > +   */
> > +  this._closeConnection = connection.request.hasHeader("Connection")
> 
> Connection closing doesn't seem to me to belong in Response code; it really
> belongs at the connection level.  Sure, you need to know what kind of
> Connection header to write out in the response, if one is needed.  But it
> seems to me the Response code should query the connection to determine how
> to do that, and shouldn't itself store that information.
> 

Please see Honza’s reply in comment#16. I prefer to stick to Honza’s original design - keep |_closeConnection| in Response code. If you still think this should be in Connection, I am happy to modify.

> @@ +3424,5 @@
> > +  this._closeConnection = connection.request.hasHeader("Connection")
> > +                        ? connection.request.getHeader("Connection")
> > +                          .toLowerCase() == "close"
> > +                        : connection.request._httpVersion.equals(
> > +                          nsHttpVersion.HTTP_1_0);
> 
> Looking at RFC2616, it looks like the Connection header is a comma-separated
> token list, so you should split and check for "close".
> 

Done.

> Do you have a citation for the comparison being case-insensitive?  I'd
> prefer an exact, case-sensitive match if nothing says otherwise, on general
> be-strict-in-what-you-accept principles.
> 
> @@ +3608,5 @@
> >      this._ensureAlive();
> >  
> >      dumpn("*** processing connection " + this._connection.number + " async");
> >      this._processAsync = true;
> > +    this._closeConnection = true;
> 
> (This looks to be in processAsync, if it's not then this might not apply.)
> 
> I don't immediately see why it should be impossible for async responses to
> work with keep-alive.  As long as they affirmatively indicate when the
> response is done -- which they do, as I recall -- it should be possible to
> make this work.  So at the very least file a followup bug on this, and put
> an XXX comment here.  (Unless all this gets resolved in the later patches in
> this bug -- I haven't looked at them at all.  :-) )
> 

As Honza explained in comment#16.

> @@ +3659,5 @@
> >          input.readByteArray(avail);
> >      }
> >  
> >      this._powerSeized = true;
> > +    this._closeConnection = true;
> 
> Since seizing power means you get to write out any sort of drek whatsoever,
> including drek that is completely malformed HTTP response data, this is very
> much appropriate.  I would add a comment clarifying that to the reader.
> 

As Honza explained in comment#16.

> @@ +3807,5 @@
> >    abort: function(e)
> >    {
> >      dumpn("*** abort(<" + e + ">)");
> >  
> > +    this._closeConnection = true;
> 
> Quite sensible.  I would add a comment here noting that because any of a
> variety of errors might have caused this, we can't know that the connection
> (either half) is in a consistent state to attempt to read another request,
> and that we give up entirely on this connection to avoid any further
> confusion.
>

Done.
 
> @@ +3851,5 @@
> >  
> > +    if (this._closeConnection)
> > +      this._connection.close();
> > +    else
> > +      this._connection.read(); /* restart reading this keep-alive connection */
> 
> These four lines should be in a method on Connection.prototype, I think -- a
> method called responseFinished() or something.  Again, whether the
> connection gets closed or the next request gets read should be left to
> Connection code, not to Response code.
> 

Done.

> @@ +3920,5 @@
> > +    {
> > +      // If "Connection: close" header had been manually set on the response,
> > +      // then set our _closeConnection flag, otherwise leave it as is.
> > +      this._closeConnection = this._closeConnection ||
> > +        (headers.getHeader("Connection").toLowerCase() == "close");
> 
> This should propagate up to the Connection code here, of course.
> 

As Honza explained in comment#16.

> @@ +3926,5 @@
> > +    else
> > +    {
> > +      // There is no Connection header set on the response, set it by state
> > +      // of the _closeConnection flag.
> > +      var connectionHeaderValue = (this._closeConnection)
> 
> And this would query the Connection for whether the connection should be
> closed, then set the appropriate header.

As Honza explained in comment#16.


(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #17)
> Comment on attachment 549788 [details] [diff] [review]
> part2: v1 - support for https schema hosts
> 
> Review of attachment 549788 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was never particularly happy with this system of inferring a port number,
> and I'm still not particularly happy with it.  But I still can't think of
> something much better here.  :-\
> 
> ::: netwerk/test/httpserver/httpd.js
> @@ +1598,5 @@
> >          // requested URI be absolute (and thus contain the necessary
> >          // information), let's assume HTTP will prevail and use that.
> > +        // _connection._incomingPort is defaulted to 80 (HTTP) but can be
> > +        // overwritten by the first tunneled request with a full specified URL.
> > +        // See also RequestReader._parseRequestLine method.
> 
> RequestReader.prototype._parseRequestLine, please.
> 

Done.

> @@ +1767,5 @@
> >  
> >        if (!serverIdentity.has(scheme, host, port) || fullPath.charAt(0) != "/")
> >          throw HTTP_400;
> > +
> > +      // Remember the port number we have determined.  Subsequest requests
> 
> Subsequent

Done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #18)
> Comment on attachment 549792 [details] [diff] [review]
> part3: v1 - chunked encoding support
> 
> Review of attachment 549792 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little leery of the way the chunking is finished off with the empty
> chunk.  And I'd like to see the final wait, which is currently try-caught
> with all exceptions swallowed, better explained.  But this seems pretty
> decent otherwise.  Still going through the others, tho, so leaving this for
> now given that these concerns don't quite seem negligible to me...
> 
> ::: netwerk/test/httpserver/httpd.js
> @@ +3510,5 @@
> >    this._processAsync = false;
> >  
> >    /**
> > +   * Flag indicating use of the chunked encoding to send the asynchronously
> > +   * generated content.  Set as an argument to processAsync method.
> 
> True if this response's asynchronously-generated content is being
> transferred using the chunked Transfer-Encoding.  Note that when the content
> of a chunked response is completely sent, this reverts to false to permit
> the final, empty chunk to be written.
> 

Yes, _chunked is set to false in |_doneReadingSource|.

> @@ +3708,5 @@
> > +
> > +    // If we are using chunked encoding then ensure body streams are present
> > +    // to send the EOF chunk, for what WriteThroughCopier is responsible.
> > +    if (this._chunked)
> > +      this.bodyOutputStream;
> 
> Hmm.  This is a bit of a kludge.  Probably we should isolate the
> create-an-output-stream logic from the bodyOutputStream getter and call it
> directly here.  For now this is probably good enough, tho.
> 

I did nothing here.

> @@ +4170,5 @@
> >   *   context passed to observer when notified of start/stop
> >   * @throws NS_ERROR_NULL_POINTER
> >   *   if source, sink, or observer are null
> >   */
> > +function WriteThroughCopier(source, sink, observer, context, chunked)
> 
> |chunked| should be documented just above, in the method description.
> 

Done.

> WriteThroughCopier was originally intended to give specific write-through
> semantics in case of error.  I'm not entirely sure what I think about
> overloading it with this new behavior.  But it seems reasonable enough to
> run with for now.
> 
> @@ +4188,5 @@
> >    /** Context for the observer watching this. */
> >    this._context = context;
> >  
> > +  /** Forces use of chunked encoding on the data */
> > +  this._chunked = chunked;
> 
> Since this gets called with incomplete arguments sometimes, this value is
> either boolean or undefined, depending.  Make this |Boolean(chunked)|
> instead to reduce the possible values this can have.
> 

Done.

> @@ +4301,5 @@
> >          bytesConsumed = data.length;
> > +
> > +        if (this._chunked)
> > +        {
> > +          // Apply chunked encoding here
> 
> As in normalizeFieldValue, please copy the ABNF for Chunked-Body here, and
> note that it came from section 3.6.1.  It's been awhile since I looked at
> RFC 2616 much, so my natural first thought was, "Does this match the spec?" 
> Copying in the ABNF makes that fairly clear to the reader.
> 

Done.

> @@ +4584,5 @@
> > +    if (this._chunked)
> > +    {
> > +      // Write the final EOF chunk
> > +      dumpn("*** _doneReadingSource - write EOF chunk");
> > +      this._chunked = false;
> 
> Hmm.  So I guess you're unsetting chunked here so that when we reenter
> onOutputStreamReady, we don't double-encode this.  And you couldn't write an
> empty string or something because that wouldn't pass the bytesRead > 0
> check.  I can't help think that writing out the trailer should really be in
> some sort of copying-finished code of some sort, not redirecting back
> through the same copy-to-sink code used earlier.  But I guess that would run
> into the async mess of wondering what would happen if the sink blocked doing
> the write, or between each byte, or somewhat.
> 
> Async is hard.  :-\
> 

Not sure what have to be done here…so I did nothing.

> @@ +4591,5 @@
> > +      {
> > +        this._waitToWriteData();
> > +        return;
> > +      }
> > +      catch (e) {}
> 
> What error are you catching here?  If there's some specific error you're
> deliberately swallowing, a comment is in order.  If there's not, um, a
> comment is still probably in order.

Done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #23)
> Comment on attachment 549795 [details] [diff] [review]
> part4: v1 - pipelining
> 
> Review of attachment 549795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am at best ambivalent about processing multiple incoming requests on the
> same connection simultaneously (if only writing them out sequentially).  But
> again, I don't really want to keep you held up for a couple weeks while I
> disappear, so I am somewhat warily +ing this.
> 

I am skeptical about whether we need to support pipelining here, but I still keep this part in the patch.
I’ll ask Honza if we can remove this part.

> ::: netwerk/test/httpserver/httpd.js
> @@ +1163,5 @@
> > +   * The current responses queue.  Each new response is pushed to this queue
> > +   * and shifted out when done.  This way we keep track of responses to process
> > +   * and let process request(s) sooner the previous response(s) were sent out.
> > +   */
> > +  this._responseQueue = [];
> 
> This has me leery.  I guess it's sort of the client's problem to be careful
> about not having too many responses written ahead of the server, in general
> (because it doesn't know how many the server might have received, or what
> side effects might have been performed).  Still, I would probably feel
> slightly easier if incoming processing stopped when a full request was
> received and only resumed when the full response to it had been generated.
> 
> (Or I'm reading this all wrong, and I shouldn't really be trying to churn
> through this review as fast as I can before I go, anyway.  :-\ )
> 

I think this was explained in comment#11.

> @@ +1223,5 @@
> >     */
> >    process: function(request)
> >    {
> > +    NS_ASSERT(!this._closed);
> > +    NS_ASSERT(request == this.request);
> 
> ===.  Really, never use ==.  The conflating it does and the potential side
> effects it might trigger make it a bad idea.
> 

Done.

> @@ +1229,5 @@
> >      this.server._handler.handleResponse(this);
> > +
> > +    this.request = null;
> > +    if (this.isIdle())
> > +      this.server._connectionIdle(this);
> 
> This temporary association between connection and request feels like it's
> tying too much logic into the Connection object to me.  I originally
> introduced Connection as a way to encapsulate only a fairly small bit of the
> nuts and bolts and mechanics of the socket transport for a connection. 
> Right now it doesn't do much more than consolidate connection closing and
> hand off a request to be handled.  I'm not sure I like stuffing particular
> request tie-in into it.  :-\
> 

I am not sure what to do right now. Maybe create a follow-up bug to improve?

> @@ +1259,5 @@
> >     * Return true if the connection is currently not receiving nor sending
> >     */
> >    isIdle: function()
> >    {
> > +    return !this._responseQueue.length && !this.request;
> 
> Compare === 0 and === null, please.
> 

Done.

> @@ +1274,5 @@
> > +   */
> > +  queueResponse: function(response)
> > +  {
> > +    dumpn("*** queueResponse on connection " + this);
> > +    if (this._responseQueue.length)
> 
> > 0
> 

Done.

> @@ +1295,5 @@
> > +    NS_ASSERT(this._responseQueue[0] == response,
> > +              "Shifting response not first in the queue??");
> > +    this._responseQueue.shift();
> > +
> > +    if (this._responseQueue.length)
> 
> > 0
> 

Done.

> @@ +2379,5 @@
> > +    // Now start reading from the connection again regardless the current
> > +    // response processing state.  The following request will be read
> > +    // and a response will be processed but will be blocked from sending to
> > +    // the connection until the current response finishes being completely
> > +    // sent out.
> 
> This is tricky, generating responses to requests before previous responses
> have been sent.  I would rather not see this complexity without a compelling
> reason for it.  I'm not sure I see one now.  Why not just process a request,
> generate the response, then read the next request, then generate the next
> responses, and so on?
> 

I believe this is for pipelining.

> @@ +3621,5 @@
> >    this._powerSeized = false;
> > +
> > +  /**
> > +   * Gets set to true if this is not the first response pending on the current
> > +   * connection.  This flag is used to block this response from writting when
> 
> "writing"
> 

Done.

> @@ +4059,5 @@
> > +    }
> > +
> > +    if (this._writeBlocked)
> > +    {
> > +      dumpn("*** response is blocked from writting");
> 
> "writing"

Done.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #22)
> Comment on attachment 549797 [details] [diff] [review]
> part5: v1 - keep-alive timeout
> 
> Review of attachment 549797 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am kind of +ing this against my better judgment in the interests of not
> blocking you for a couple weeks while I'm gone.  Not your fault, of course
> -- just the happenstance of when I had my vacation planned.  And probably I
> deserve a little blame for not getting to this review sooner, to allow extra
> turnaround time on responding to reviews.  :-\
> 
> ::: netwerk/test/httpserver/httpd.js
> @@ +404,5 @@
> > +   * Flag for keep-alive behavior, true for default behavior, false for
> > +   * closing connections after the first response.
> > +   * Set by nsIHttpServer.keepAlive attribute.
> > +   */
> > +  this._defaultKeepAlive = true;
> 
> This shouldn't be named with "default" in the name: it kind of makes me
> think that its value is a constant (notwithstanding context).  But
> "_keepAlive" also sounds verb-y, not like a boolean field.  How about
> "keepAliveEnabled" (name the attribute in IDL "keepAliveEnabled", name the
> property "_keepAliveEnabled") instead?
> 

Done.

> It's not necessary to call out what attribute gets/sets a property,
> generally -- particularly if you make the naming change I suggest.
> 
> @@ +407,5 @@
> > +   */
> > +  this._defaultKeepAlive = true;
> > +
> > +  /**
> > +   * The number of seconds to keep idle persistent connections alive
> 
> I think you could collapse this to a single line /** ... */ comment without
> going over 80 characters, but I'm only eyeballing it.
> 

Done.

> @@ +409,5 @@
> > +
> > +  /**
> > +   * The number of seconds to keep idle persistent connections alive
> > +   */
> > +  this._defaultKeepAliveTimeout = 120;
> 
> 2 * 60 might be slightly nicer to read.
> 

Done.

> @@ +716,5 @@
> > +    return this._defaultKeepAlive;
> > +  },
> > +  set keepAlive(doKeepAlive)
> > +  {
> > +    return this._defaultKeepAlive = doKeepAlive;
> 
> The value returned by a setter function is, for better or worse, ignored. 
> So this shouldn't return a value.
> 

Done.

> @@ +813,5 @@
> >        this._notifyStopped();
> >      // Bug 508125: Add a GC here else we'll use gigabytes of memory running
> >      // mochitests. We can't rely on xpcshell doing an automated GC, as that
> >      // would interfere with testing GC stuff...
> > +    if (typeof gc === "function") gc();
> 
> if (...)
>   gc();
> 

Done.

> @@ +822,5 @@
> >     * may be closed when the server went down.
> >     */
> >    _connectionIdle: function(connection)
> >    {
> >      // If the server is down, close any now idle connections.
> 
> "now-idle", as the two words together form an adjective for "connections".
> 

Done.

> @@ +1200,5 @@
> >    this._responseQueue = [];
> > +
> > +  /**
> > +   * The current keep-alive timeout value.  Adjustable by Keep-alive: timeout
> > +   * header in requests and responses.  Units: seconds.
> 
> "The current keep-alive timeout duration, in seconds.  Individual
> requests/responses can modify this value through the Keep-Alive header."
> 

Done.

> @@ +1205,5 @@
> > +   */
> > +  this._keepAliveTimeout = server._defaultKeepAliveTimeout;
> > +
> > +  /**
> > +   * Start the initial timeout, if no data are read on this connection within
> 
> Period after timeout, because the first three words are a distinct sentence,
> as are the remaining words of this comment.
> 

Done.

> "Data" is the plural of "datum", to be sure.  But it is also a perfectly
> valid collective noun in its own right.  And it reads much better when used
> that way (at least here), too.  So make this "If no data is read from...".
> 

Done.

> @@ +1264,5 @@
> >        server.stop(function() { /* not like we can do anything better */ });
> >    },
> >  
> > +  /** Let the connection be persistent for the keep-alive time */
> > +  persist: function()
> 
> I'm pretty sure we could do the cancel-and-reinit thing pretty nicely, but
> this is already written, and I'm running out of time to review a patch which
> did it better.  I'm still pretty sure that's possible, notwithstanding your
> comments here.  Maybe some other time.
> 
> It would be nice if you abstracted the cancel-and-null bit into a method,
> tho, to centralize that logic just a little bit more.
> 

Not sure about “cancel-and-reinit” and “cancel-and-null” you mean. Could you explain more?

> @@ +1339,5 @@
> >        this.server._connectionIdle(this);
> >    },
> >  
> >    /**
> > +   * Return true if the connection is currently not receiving nor sending.
> 
> This probably doesn't matter given the comment further down, but for future
> reference: "not...or...", not "not...nor...".
> 

Done.

> @@ +1346,2 @@
> >     */
> >    isIdle: function()
> 
> The description should describe the semantics it implements.  It doesn't
> quite do this right now.  How about this instead:
> 
> "Returns true iff this connection is not closed and is idle.  A connection
> is idle when all requests received on it have triggered responses, and those
> responses have been completely sent."
> 

Done.

> @@ +1346,4 @@
> >     */
> >    isIdle: function()
> >    {
> > +    return !this._responseQueue.length && !this.request && !this._closed;
> 
> length === 0, still.
> 

Done.

> @@ +4202,3 @@
> >        // There is no Connection header set on the response, set it by state
> >        // of the _closeConnection flag.
> >        var connectionHeaderValue = (this._closeConnection)
> 
> I don't remember which patch added this (or even if it was that very first
> patch that I reviewed, offhand), but the condition should not be
> parenthesized.
> 

Done.

> @@ +4206,5 @@
> >                                  : "keep-alive";
> >        headers.setHeader("Connection", connectionHeaderValue, false);
> >      }
> >  
> > +    if (headers.hasHeader("Keep-alive"))
> 
> Keep-Alive is a more common capitalization, so use that.
> 

Done.

> @@ +4210,5 @@
> > +    if (headers.hasHeader("Keep-alive"))
> > +    {
> > +      // Read the Keep-alive header and set the timeout according it.
> > +      var keepAliveTimeout = headers.getHeader("Keep-alive")
> > +                             .match(/timeout\s*=\s*(\d+)/);
> 
> /^...$/ to be more restrictive about the exact syntax we demand, please.  I
> don't see a whole lot of value in supporting the 0# bit that RFC 2068
> mentions.
> 

Done.

> Speaking of which, you should mention in a comment that this header is
> defined by RFC 2068, and perhaps mention that it's no longer in RFC 2616,
> maybe with a reference to the explanation why.
> 

Done.

> @@ +4212,5 @@
> > +      // Read the Keep-alive header and set the timeout according it.
> > +      var keepAliveTimeout = headers.getHeader("Keep-alive")
> > +                             .match(/timeout\s*=\s*(\d+)/);
> > +      if (keepAliveTimeout)
> > +        this._connection._keepAliveTimeout = parseInt(keepAliveTimeout[1]);
> 
> parseInt without an explicit radix is dangerous.  Always explicitly specify
> a radix (10 here) when using parseInt.
> 

Done.

> ::: netwerk/test/httpserver/nsIHttpServer.idl
> @@ +242,5 @@
> > +   *        and announce it with Connection: close header.
> > +   *
> > +   * Default value is TRUE.
> > +   */
> > +  attribute PRBool keepAlive;
> 
> The description here is a bit unusual as far as formatting goes.  I suggest
> something like this:
> 
> Configures use of keep-alive connections, wherein the same network
> connection may be used to receive and respond to multiple requests.  If
> true, keep-alive connections will be used when possible.  If false,
> keep-alive connections will not be used, and responses will include a
> "Connection: close" header.  Defaults to true.
> 

Done.

> Ideally I'd prefer slightly more detail concerning "when possible", but I
> don't actually remember keep-alive semantics all that well right now, and
> given that I'm going to be gone fairly shortly and have many other things to
> do (foremost among them clear out my review queue to not block anyone else,
> ideally), I don't really have the time to spend to investigate.  :-\  Also,
> I'm not sure what gradation of detail I'd want to include here.

Comment 34

[Kershaw Chang [:kershaw]]

Honza,

I would like to ask you some questions about httpd.js.
1. Do we still need to support pipelining? Can I remove this part?

2. I encountered some test failures (mochitest and reftests) on try server and I have no idea why these failures are correlated to httpd.js. I'd like to know in your previous experience how did you debug this kind of failure? Is it possible to disable keep-alive for some tests?
It seems that this is related to the keep-alive timeout value and max connections in nsIServerSocket, but I am not sure.

Thanks.

Comment 35

[Honza Bambas (:mayhemer)]

(In reply to Kershaw Chang [:kershaw] from comment #34)
> Honza,
> 
> I would like to ask you some questions about httpd.js.
> 1. Do we still need to support pipelining? Can I remove this part?

Glad you asked.  Yes, remove it.  We are no longer supporting it.

> 
> 2. I encountered some test failures (mochitest and reftests) on try server
> and I have no idea why these failures are correlated to httpd.js. I'd like
> to know in your previous experience how did you debug this kind of failure?

Individually :)  This is because some tests are not ready for keep-alive and need update.  I have experienced this before when I was writing this patch at those good old times, some tests had to be fixed, see the "Depends on" list of bugs for example patches.

> Is it possible to disable keep-alive for some tests?

The server should respect Connection: close request header (not sure I've implemented it or not in the patch here) so, when there is no other way, we can use it in the tests explicitly.  But I'm afraid that e.g. mochitests making requests through some other APIs like XHR or fetch() will not be able to be configured with that header.  I think better is to put some reasonable effort to fix the individual failing tests.

> It seems that this is related to the keep-alive timeout value and max
> connections in nsIServerSocket, but I am not sure.

Run the failing test w/o keep alive and with it and see the difference.  That is usually the way I investigate these kinds of failures.  You can also add dumps to the test to see how it passes, use logging etc.

If you get totally lost, I can help.

> 
> Thanks.

Comment 36

[Kershaw Chang [:kershaw]]

Comment on attachment 8834792 [details] [diff] [review]
All parts: keep-alive, chunked and pipelining support

Review of attachment 8834792 [details] [diff] [review]:
-----------------------------------------------------------------

Cancel the review now since pipelining should be removed.

Comment 37

[Kershaw Chang [:kershaw]]

Attachment: Support keep-alive and chunked encoding

Summary:
 - Remove pipelining
 - Only enable chunked encoding when content-length is unavailable


Not going to review since this sill pose some test failures (mostly are shutdown leaks).

Comment 38

[Kershaw Chang [:kershaw]]

Attachment: Part1: Support keep-alive

Summary:
 - Support keep-alive in httpd.js

Hi Jeff,

Could you take a look at this patch? Thanks.

Some tests are green now after retrying.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=970241dd02a8d8ef4d1ac5c413b561c90a5111c9&group_state=expanded

Comment 39

[Kershaw Chang [:kershaw]]

Attachment: Part2: Modify failed tests

Summary:
 - Modify tests to support keep-alive
 - Disable keepAlive for some tests

Not sure if I should ask the original author or you to review this.
What do you think?

Comment 40

[Kershaw Chang [:kershaw]]

Attachment: Part3: Close all connections at the end

Summary:
 - Close all active connections before collecting memory leak report
 - If we don't do this, some http connection objects would be considered as a part of memory leak.

Thanks.

Comment 41

[Jeff Walden [:Waldo]]

Comment on attachment 8861860 [details] [diff] [review]
Part2: Modify failed tests

Review of attachment 8861860 [details] [diff] [review]:
-----------------------------------------------------------------

The two issues mentioned below aside, this all looks fine.  r=me in the interests of time, but *only* on the assumption that the first issue will be dealt with when landing, and the second issue will be dealt with in another patch before landing.

::: netwerk/test/unit/test_bug561042.js
@@ +15,5 @@
>      cookie += " big cookie";
>  }
>  
> +var server = null;
> +

Why is this necessary or desirable?  If this is some sort of semi-dumb linting requirement, move the declaration/assignment of |server| 15 lines down up to here.

::: toolkit/mozapps/extensions/test/browser/redirect.sjs
@@ +1,5 @@
>  function handleRequest(request, response) {
>    dump("*** Received redirect for " + request.queryString + "\n");
>    response.setStatusLine(request.httpVersion, 301, "Moved Permanently");
>    response.setHeader("Location", request.queryString, false);
> +  response._closeConnection = true;

Umm, no.  Not acceptable to assert an existing connection must be closed via backdoor implementation-internal property like this.  Please create a separate patch adding a closeConnection() function and use that instead.

Comment 42

[Jeff Walden [:Waldo]]

Comment on attachment 8861857 [details] [diff] [review]
Part1: Support keep-alive

Review of attachment 8861857 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/httpserver/httpd.js
@@ +399,5 @@
> +   */
> +  this._keepAliveEnabled = true;
> +
> +  /** The number of seconds to keep idle persistent connections alive. */
> +  this._defaultKeepAliveTimeout = 2 * 60;

Make this a global const, please.

@@ +857,5 @@
>    },
>  
>    /**
> +   * Inform the server that the connection is currently in an idle state and
> +   * may be closed when the server went down.

s/went/goes/

@@ +1221,5 @@
> +  this._requestStarted = false;
> +
> +  /**
> +   * RequestReader may cache the port number here.  This is needed because when
> +   * going thought ssltunnel, we update the Request-URL only for the first

through

@@ +1222,5 @@
> +
> +  /**
> +   * RequestReader may cache the port number here.  This is needed because when
> +   * going thought ssltunnel, we update the Request-URL only for the first
> +   * request, through we get knowledge of the actual target server port number.

"through which"?  I'm not entirely sure how to read this.

@@ +1223,5 @@
> +  /**
> +   * RequestReader may cache the port number here.  This is needed because when
> +   * going thought ssltunnel, we update the Request-URL only for the first
> +   * request, through we get knowledge of the actual target server port number.
> +   * Following requests are not updated that way and are missing the port number

"""
Subsequent requests aren't 
"""

@@ +1309,5 @@
> +    // i.e. has sent all queued responses out and doesn't process a request now.
> +    if (this._idleTimer)
> +    {
> +      this._idleTimer.cancel();
> +      this._idleTimer = null;

Timers can be reused, so you don't have to be constantly creating them.  Given that, this is the wrong way to structure all this code.  Rather:

  var idleTimer = this._idleTimer;
  if (idleTimer)
    idleTimer.cancel();
  else
    this._idleTimer = idleTimer = new Timer();

  var connection = this;
  idleTimer.initWithCallback(function()
  {
    ...
  }, this._keepAliveTimeout * 1000, Ci.nITimer.TYPE_ONE_SHOT);

with the appropriate comments'n'such inserted in it.

@@ +1316,5 @@
> +    dumpn("*** persisting idle connection " + this +
> +          " for " + this._keepAliveTimeout + " seconds");
> +
> +    this._idleTimer = Components.classes["@mozilla.org/timer;1"]
> +                      .createInstance(Components.interfaces.nsITimer);

Please add

  const Timer = CC("@mozilla.org/timer;1", "nsITimer");

to the section full of other CC(...) calls elsewhere in this file, then use |new Timer()| here.

@@ +1317,5 @@
> +          " for " + this._keepAliveTimeout + " seconds");
> +
> +    this._idleTimer = Components.classes["@mozilla.org/timer;1"]
> +                      .createInstance(Components.interfaces.nsITimer);
> +    this._idleTimer.target = gThreadManager.mainThread;

Don't do this -- this JS only runs on the main thread, so just having the default target -- "By default the target is the thread that created the timer." -- should be fine.

@@ +1323,5 @@
> +    var connection = this;
> +    this._idleTimer.initWithCallback(function()
> +    {
> +      // We might get to an active state before the timeout occurred, then
> +      // ignore it.  Timer will get rescheduled when connection gets back

"The timer will be rescheduled when the connection returns to the idle state."

@@ +1325,5 @@
> +    {
> +      // We might get to an active state before the timeout occurred, then
> +      // ignore it.  Timer will get rescheduled when connection gets back
> +      // to the idle state.  This is simpler and more error-prone then canceling
> +      // the timer whenever the connection becomes active again.

*less* error-prone?

@@ +1379,5 @@
> +   */
> +  isIdle: function()
> +  {
> +    return this.request === null &&
> +           !this._closed;

This fits on one line.

@@ +1735,5 @@
>          // scheme to use to look up the correct port here, in general.  Since
>          // the HTTPS case requires a tunnel/proxy and thus requires that the
>          // requested URI be absolute (and thus contain the necessary
>          // information), let's assume HTTP will prevail and use that.
> +        // _connection._incomingPort is defaulted to 80 (HTTP) but can be

s/is defaulted to/defaults to/

@@ +1737,5 @@
>          // requested URI be absolute (and thus contain the necessary
>          // information), let's assume HTTP will prevail and use that.
> +        // _connection._incomingPort is defaulted to 80 (HTTP) but can be
> +        // overwritten by the first tunneled request with a full specified URL.
> +        // See also RequestReader.prototype._parseRequestLine method.

s/ method//

@@ +1928,5 @@
>        }
> +
> +      // Remember the port number we have determined. Subsequent requests
> +      // might not contain this information.
> +      this._connection._incomingPort = port;

Could you rename this to _currentIncomingPort, to more clearly identify that this value can change over time?

@@ +3688,5 @@
> +   */
> +  var req = connection.request;
> +  this._closeConnection = !connection.server._keepAliveEnabled ||
> +                          (req.hasHeader("Connection")
> +                          ? req.getHeader("Connection").split(",").indexOf("close") !== -1

.includes("close")

@@ +3689,5 @@
> +  var req = connection.request;
> +  this._closeConnection = !connection.server._keepAliveEnabled ||
> +                          (req.hasHeader("Connection")
> +                          ? req.getHeader("Connection").split(",").indexOf("close") !== -1
> +                          : req._httpVersion.equals(nsHttpVersion.HTTP_1_0));

Use |!req._httpVersion.atLeast(nsHttpVersion.HTTP_1_1)| please.

@@ +3956,5 @@
>      dumpn("*** finishing connection " + this._connection.number);
>      this._startAsyncProcessor(); // in case bodyOutputStream was never accessed
> +
> +    // If we are using chunked encoding then ensure body streams are present
> +    // to send the EOF chunk, for what WriteThroughCopier is responsible.

...are present so that WriteTrhoughCopier will send an EOF chunk.

@@ +4205,5 @@
> +    {
> +      // If "Connection: close" header had been manually set on the response,
> +      // then set our _closeConnection flag, otherwise leave it as is.
> +      this._closeConnection = this._closeConnection ||
> +        (headers.getHeader("Connection").split(",").indexOf("close") !== -1);

.includes("close"), and no surrounding parentheses.

@@ +4224,5 @@
> +      // Note: This is documented in RFC 2068, section 19.7.1.
> +      var keepAliveTimeout = headers.getHeader("Keep-Alive")
> +                             .match(/^timeout=(\d+)$/);
> +      if (keepAliveTimeout)
> +        this._connection._keepAliveTimeout = parseInt(keepAliveTimeout[1], 10);

Add some error-handling for very-large keep-alive values -- maybe

  if (keepAliveTimeout)
  {
    var seconds = parseInt(keepAliveTimeout[1], 10);
    this._connection._keepAliveTimeout = Math.min(seconds, MAX_KEEPALIVE_TIMEOUT;
  }

with a

  const MAX_KEEPALIVE_TIMEOUT = 60; // one minute

elsewhere in the file.  (Of course note that \d+ guarantees the number will always parse -- it `just might be unusably large or even infinite, but never NaN.  So a minimum is adequate safeguard.)

@@ +4256,5 @@
>        headers.setHeader("Content-Length", "" + avail, false);
>      }
>  
> +    if (this._headers.hasHeader("Content-Length")) {
> +      this._chunked = false;

this._chunked = !this._headers.hasHeader("Content-Length");

@@ +4264,5 @@
> +    dumpn("*** this._chunked= " + this._chunked);
> +    if (this._chunked)
> +    {
> +      headers.setHeader("Transfer-Encoding", "chunked", false);
> +    }

Don't brace a single-line if.

@@ +4394,5 @@
>  
>      dumpn("*** starting async copier of body data...");
>      this._asyncCopier =
>        new WriteThroughCopier(this._bodyInputStream, this._connection.output,
> +                            copyObserver, null, this._chunked);

Since you're touching this, indent one further space to align "this" with "copyObserver".

@@ +4465,5 @@
>    /** Context for the observer watching this. */
>    this._context = context;
>  
> +  /** Forces use of chunked encoding on the data */
> +  this._chunked = Boolean(chunked);

Pass this through without change -- it's on the callers to pass in actual boolean values.  So add |false| to the one other caller outside this patch's context, and to netwerk/test/httpserver/test/test_async_response_sending.js.

@@ +4604,5 @@
> +          this._pendingData.push(data);
> +        }
> +        else
> +        {
> +          this._pendingData.push(String.fromCharCode.apply(String, data));

Please extract out |String.fromCharCode.apply(...)| into a local variable that both arms can use.

@@ +4887,5 @@
> +        return;
> +      }
> +      catch (e)
> +      {
> +        dumpn("!!! unexpected error waiting to write pending data: " + e);

I'm not certain on the error-handling here, and I'm running out of time to re-figure it out.  So for now, please rethrow the error rather than return silently?

::: netwerk/test/httpserver/nsIHttpServer.idl
@@ +226,5 @@
> +   * keep-alive connections will not be used, and responses will include a
> +   * "Connection: close" header.
> +   * Defaults to true.
> +   */
> +  attribute boolean keepAliveEnabled;

Adding IDL attributes or functions requires bumping the UUIDs on each interface you touch.  (And on any interface that inherits from them -- probably there are none here, but do a search for the interface names to be sure.)  Please do that -- "firebot: uuid" on IRC will give you fresh ones.

A part of me wonders whether we shouldn't make this different: not a boolean indicating keep-aliveness, but a number indicating the keep-alive duration, with 0 meaning no-keep-alive.  But I might be overthinking it on trying to cabin complexity, especially as I'm not certain that storing keep-alive: 0 as an attribute like this would be exactly identical to having it off entirely.

@@ +590,5 @@
>     *   called after this response has been finished
>     * @throws NS_ERROR_NOT_AVAILABLE
>     *   if seizePower() has been called on this
>     */
> +  void processAsync(in boolean useChunkedEncoding);

Seems to me this belongs in a different patch in this set (although I guess they're going to be folded together, so it's fine).

Or -- looking a little harder at the patch, you haven't changed processAsync's code at all to set this, so isn't the chunked-coding code in this patch effectively dead now?  |this._chunked| is set entirely depending on whether the response headers contain Content-Length or not.  So this change should go away, right?

Comment 43

[Jeff Walden [:Waldo]]

Some good news and some bad news.

Good news first: you have reviews here! most of the issues are nits I don't need to look at again!

Bad news: I really would like to see the closeConnection() patch because of its meaning IDL changes and *particularly* that means a good documentation comment must be written.  And the really unfortunate thing is, I leave on a *very* extended vacation starting next Monday, and it's entirely unrealistic for the patches up for my review here to be delayed til my return.

However, while vacation starts Monday, it doesn't start with a vengeance until the night of May 9.  I'm busy Monday afternoon and maybe some in the morning, but otherwise I should have *some* amount of time to finish a fairly fast review here.

So: if you can get the important, non-typo-y bits of change into a patch for me to review very early next week, I will do my utmost to review it before I'm gone.  I'm currently not accepting new reviews in Bugzilla -- so instead of getting a flag, please just attach the patch, then send me an email telling me about it (in case I don't notice it in bugmail).  Bugzilla email address minus the +bmo bit should work.

Comment 44

[Kershaw Chang [:kershaw]]

(In reply to Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important) from comment #43)
> Some good news and some bad news.
> 
> Good news first: you have reviews here! most of the issues are nits I don't
> need to look at again!
> 
> Bad news: I really would like to see the closeConnection() patch because of
> its meaning IDL changes and *particularly* that means a good documentation
> comment must be written.  And the really unfortunate thing is, I leave on a
> *very* extended vacation starting next Monday, and it's entirely unrealistic
> for the patches up for my review here to be delayed til my return.
> 
> However, while vacation starts Monday, it doesn't start with a vengeance
> until the night of May 9.  I'm busy Monday afternoon and maybe some in the
> morning, but otherwise I should have *some* amount of time to finish a
> fairly fast review here.
> 
> So: if you can get the important, non-typo-y bits of change into a patch for
> me to review very early next week, I will do my utmost to review it before
> I'm gone.  I'm currently not accepting new reviews in Bugzilla -- so instead
> of getting a flag, please just attach the patch, then send me an email
> telling me about it (in case I don't notice it in bugmail).  Bugzilla email
> address minus the +bmo bit should work.

Thanks!
I'll send the patch to you by Monday morning.

Comment 45

[Kershaw Chang [:kershaw]]

(In reply to Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important) from comment #41)
> Comment on attachment 8861860 [details] [diff] [review]
> Part2: Modify failed tests
> 
> Review of attachment 8861860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The two issues mentioned below aside, this all looks fine.  r=me in the
> interests of time, but *only* on the assumption that the first issue will be
> dealt with when landing, and the second issue will be dealt with in another
> patch before landing.
> 
> ::: netwerk/test/unit/test_bug561042.js
> @@ +15,5 @@
> >      cookie += " big cookie";
> >  }
> >  
> > +var server = null;
> > +
> 
> Why is this necessary or desirable?  If this is some sort of semi-dumb
> linting requirement, move the declaration/assignment of |server| 15 lines
> down up to here.
> 

This is a dumb mistake happened due to rebase.
We don't need this change.

> ::: toolkit/mozapps/extensions/test/browser/redirect.sjs
> @@ +1,5 @@
> >  function handleRequest(request, response) {
> >    dump("*** Received redirect for " + request.queryString + "\n");
> >    response.setStatusLine(request.httpVersion, 301, "Moved Permanently");
> >    response.setHeader("Location", request.queryString, false);
> > +  response._closeConnection = true;
> 
> Umm, no.  Not acceptable to assert an existing connection must be closed via
> backdoor implementation-internal property like this.  Please create a
> separate patch adding a closeConnection() function and use that instead.

I'll do it.

Comment 46

[Kershaw Chang [:kershaw]]

(In reply to Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important) from comment #42)
> Comment on attachment 8861857 [details] [diff] [review]
> Part1: Support keep-alive
> 
> Review of attachment 8861857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/test/httpserver/httpd.js
> @@ +399,5 @@
> > +   */
> > +  this._keepAliveEnabled = true;
> > +
> > +  /** The number of seconds to keep idle persistent connections alive. */
> > +  this._defaultKeepAliveTimeout = 2 * 60;
> 
> Make this a global const, please.
> 

Done.

> @@ +857,5 @@
> >    },
> >  
> >    /**
> > +   * Inform the server that the connection is currently in an idle state and
> > +   * may be closed when the server went down.
> 
> s/went/goes/
> 

Done.

> @@ +1221,5 @@
> > +  this._requestStarted = false;
> > +
> > +  /**
> > +   * RequestReader may cache the port number here.  This is needed because when
> > +   * going thought ssltunnel, we update the Request-URL only for the first
> 
> through
> 

Done.

> @@ +1222,5 @@
> > +
> > +  /**
> > +   * RequestReader may cache the port number here.  This is needed because when
> > +   * going thought ssltunnel, we update the Request-URL only for the first
> > +   * request, through we get knowledge of the actual target server port number.
> 
> "through which"?  I'm not entirely sure how to read this.
> 

I think this should be "though".

> @@ +1223,5 @@
> > +  /**
> > +   * RequestReader may cache the port number here.  This is needed because when
> > +   * going thought ssltunnel, we update the Request-URL only for the first
> > +   * request, through we get knowledge of the actual target server port number.
> > +   * Following requests are not updated that way and are missing the port number
> 
> """
> Subsequent requests aren't 
> """
> 

Done.

> @@ +1309,5 @@
> > +    // i.e. has sent all queued responses out and doesn't process a request now.
> > +    if (this._idleTimer)
> > +    {
> > +      this._idleTimer.cancel();
> > +      this._idleTimer = null;
> 
> Timers can be reused, so you don't have to be constantly creating them. 
> Given that, this is the wrong way to structure all this code.  Rather:
> 
>   var idleTimer = this._idleTimer;
>   if (idleTimer)
>     idleTimer.cancel();
>   else
>     this._idleTimer = idleTimer = new Timer();
> 
>   var connection = this;
>   idleTimer.initWithCallback(function()
>   {
>     ...
>   }, this._keepAliveTimeout * 1000, Ci.nITimer.TYPE_ONE_SHOT);
> 
> with the appropriate comments'n'such inserted in it.
> 
> @@ +1316,5 @@
> > +    dumpn("*** persisting idle connection " + this +
> > +          " for " + this._keepAliveTimeout + " seconds");
> > +
> > +    this._idleTimer = Components.classes["@mozilla.org/timer;1"]
> > +                      .createInstance(Components.interfaces.nsITimer);
> 
> Please add
> 
>   const Timer = CC("@mozilla.org/timer;1", "nsITimer");
> 
> to the section full of other CC(...) calls elsewhere in this file, then use
> |new Timer()| here.
> 

Done.

> @@ +1317,5 @@
> > +          " for " + this._keepAliveTimeout + " seconds");
> > +
> > +    this._idleTimer = Components.classes["@mozilla.org/timer;1"]
> > +                      .createInstance(Components.interfaces.nsITimer);
> > +    this._idleTimer.target = gThreadManager.mainThread;
> 
> Don't do this -- this JS only runs on the main thread, so just having the
> default target -- "By default the target is the thread that created the
> timer." -- should be fine.
> 

This line is removed.

> @@ +1323,5 @@
> > +    var connection = this;
> > +    this._idleTimer.initWithCallback(function()
> > +    {
> > +      // We might get to an active state before the timeout occurred, then
> > +      // ignore it.  Timer will get rescheduled when connection gets back
> 
> "The timer will be rescheduled when the connection returns to the idle
> state."
> 

Done.

> @@ +1325,5 @@
> > +    {
> > +      // We might get to an active state before the timeout occurred, then
> > +      // ignore it.  Timer will get rescheduled when connection gets back
> > +      // to the idle state.  This is simpler and more error-prone then canceling
> > +      // the timer whenever the connection becomes active again.
> 
> *less* error-prone?
> 

Yes. Already fixed.

> @@ +1379,5 @@
> > +   */
> > +  isIdle: function()
> > +  {
> > +    return this.request === null &&
> > +           !this._closed;
> 
> This fits on one line.
> 

Done.

> @@ +1735,5 @@
> >          // scheme to use to look up the correct port here, in general.  Since
> >          // the HTTPS case requires a tunnel/proxy and thus requires that the
> >          // requested URI be absolute (and thus contain the necessary
> >          // information), let's assume HTTP will prevail and use that.
> > +        // _connection._incomingPort is defaulted to 80 (HTTP) but can be
> 
> s/is defaulted to/defaults to/
> 

Done.

> @@ +1737,5 @@
> >          // requested URI be absolute (and thus contain the necessary
> >          // information), let's assume HTTP will prevail and use that.
> > +        // _connection._incomingPort is defaulted to 80 (HTTP) but can be
> > +        // overwritten by the first tunneled request with a full specified URL.
> > +        // See also RequestReader.prototype._parseRequestLine method.
> 
> s/ method//
> 

Done.

> @@ +1928,5 @@
> >        }
> > +
> > +      // Remember the port number we have determined. Subsequent requests
> > +      // might not contain this information.
> > +      this._connection._incomingPort = port;
> 
> Could you rename this to _currentIncomingPort, to more clearly identify that
> this value can change over time?
> 

Done.

> @@ +3688,5 @@
> > +   */
> > +  var req = connection.request;
> > +  this._closeConnection = !connection.server._keepAliveEnabled ||
> > +                          (req.hasHeader("Connection")
> > +                          ? req.getHeader("Connection").split(",").indexOf("close") !== -1
> 
> .includes("close")
> 

Done.

> @@ +3689,5 @@
> > +  var req = connection.request;
> > +  this._closeConnection = !connection.server._keepAliveEnabled ||
> > +                          (req.hasHeader("Connection")
> > +                          ? req.getHeader("Connection").split(",").indexOf("close") !== -1
> > +                          : req._httpVersion.equals(nsHttpVersion.HTTP_1_0));
> 
> Use |!req._httpVersion.atLeast(nsHttpVersion.HTTP_1_1)| please.
> 

Done.

> @@ +3956,5 @@
> >      dumpn("*** finishing connection " + this._connection.number);
> >      this._startAsyncProcessor(); // in case bodyOutputStream was never accessed
> > +
> > +    // If we are using chunked encoding then ensure body streams are present
> > +    // to send the EOF chunk, for what WriteThroughCopier is responsible.
> 
> ...are present so that WriteTrhoughCopier will send an EOF chunk.
> 

Done.

> @@ +4205,5 @@
> > +    {
> > +      // If "Connection: close" header had been manually set on the response,
> > +      // then set our _closeConnection flag, otherwise leave it as is.
> > +      this._closeConnection = this._closeConnection ||
> > +        (headers.getHeader("Connection").split(",").indexOf("close") !== -1);
> 
> .includes("close"), and no surrounding parentheses.
> 

Done.

> @@ +4224,5 @@
> > +      // Note: This is documented in RFC 2068, section 19.7.1.
> > +      var keepAliveTimeout = headers.getHeader("Keep-Alive")
> > +                             .match(/^timeout=(\d+)$/);
> > +      if (keepAliveTimeout)
> > +        this._connection._keepAliveTimeout = parseInt(keepAliveTimeout[1], 10);
> 
> Add some error-handling for very-large keep-alive values -- maybe
> 
>   if (keepAliveTimeout)
>   {
>     var seconds = parseInt(keepAliveTimeout[1], 10);
>     this._connection._keepAliveTimeout = Math.min(seconds,
> MAX_KEEPALIVE_TIMEOUT;
>   }
> 
> with a
> 
>   const MAX_KEEPALIVE_TIMEOUT = 60; // one minute
> 
> elsewhere in the file.  (Of course note that \d+ guarantees the number will
> always parse -- it `just might be unusably large or even infinite, but never
> NaN.  So a minimum is adequate safeguard.)
> 

I did the modification you suggested. But I was wondering could we use the default keep-alive timeout (120) here?
Why do we need another value?

> @@ +4256,5 @@
> >        headers.setHeader("Content-Length", "" + avail, false);
> >      }
> >  
> > +    if (this._headers.hasHeader("Content-Length")) {
> > +      this._chunked = false;
> 
> this._chunked = !this._headers.hasHeader("Content-Length");
> 

Done.

> @@ +4264,5 @@
> > +    dumpn("*** this._chunked= " + this._chunked);
> > +    if (this._chunked)
> > +    {
> > +      headers.setHeader("Transfer-Encoding", "chunked", false);
> > +    }
> 
> Don't brace a single-line if.
> 

Done.

> @@ +4394,5 @@
> >  
> >      dumpn("*** starting async copier of body data...");
> >      this._asyncCopier =
> >        new WriteThroughCopier(this._bodyInputStream, this._connection.output,
> > +                            copyObserver, null, this._chunked);
> 
> Since you're touching this, indent one further space to align "this" with
> "copyObserver".
> 

Done.

> @@ +4465,5 @@
> >    /** Context for the observer watching this. */
> >    this._context = context;
> >  
> > +  /** Forces use of chunked encoding on the data */
> > +  this._chunked = Boolean(chunked);
> 
> Pass this through without change -- it's on the callers to pass in actual
> boolean values.  So add |false| to the one other caller outside this patch's
> context, and to netwerk/test/httpserver/test/test_async_response_sending.js.
> 

Done.

> @@ +4604,5 @@
> > +          this._pendingData.push(data);
> > +        }
> > +        else
> > +        {
> > +          this._pendingData.push(String.fromCharCode.apply(String, data));
> 
> Please extract out |String.fromCharCode.apply(...)| into a local variable
> that both arms can use.
> 

Done.

> @@ +4887,5 @@
> > +        return;
> > +      }
> > +      catch (e)
> > +      {
> > +        dumpn("!!! unexpected error waiting to write pending data: " + e);
> 
> I'm not certain on the error-handling here, and I'm running out of time to
> re-figure it out.  So for now, please rethrow the error rather than return
> silently?
> 

Agree. I'll just re-throw the exception.

> ::: netwerk/test/httpserver/nsIHttpServer.idl
> @@ +226,5 @@
> > +   * keep-alive connections will not be used, and responses will include a
> > +   * "Connection: close" header.
> > +   * Defaults to true.
> > +   */
> > +  attribute boolean keepAliveEnabled;
> 
> Adding IDL attributes or functions requires bumping the UUIDs on each
> interface you touch.  (And on any interface that inherits from them --
> probably there are none here, but do a search for the interface names to be
> sure.)  Please do that -- "firebot: uuid" on IRC will give you fresh ones.
> 

I thought we don't have to update UUID anymore when changing XPIDL interface?
Please see https://groups.google.com/forum/#!topic/mozilla.dev.platform/HE1_qZhPj1I.

> A part of me wonders whether we shouldn't make this different: not a boolean
> indicating keep-aliveness, but a number indicating the keep-alive duration,
> with 0 meaning no-keep-alive.  But I might be overthinking it on trying to
> cabin complexity, especially as I'm not certain that storing keep-alive: 0
> as an attribute like this would be exactly identical to having it off
> entirely.
> 
> @@ +590,5 @@
> >     *   called after this response has been finished
> >     * @throws NS_ERROR_NOT_AVAILABLE
> >     *   if seizePower() has been called on this
> >     */
> > +  void processAsync(in boolean useChunkedEncoding);
> 
> Seems to me this belongs in a different patch in this set (although I guess
> they're going to be folded together, so it's fine).
> 
> Or -- looking a little harder at the patch, you haven't changed
> processAsync's code at all to set this, so isn't the chunked-coding code in
> this patch effectively dead now?  |this._chunked| is set entirely depending
> on whether the response headers contain Content-Length or not.  So this
> change should go away, right?

Yes. This should go away.

Comment 47

[Jeff Walden [:Waldo]]

(In reply to Kershaw Chang [:kershaw] from comment #46)
> I did the modification you suggested. But I was wondering could we use the
> default keep-alive timeout (120) here?
> Why do we need another value?

I could imagine permitting a larger-than-default value here.  But capping at the default works as well.  So one constant is cool.

> I thought we don't have to update UUID anymore when changing XPIDL interface?
> Please see
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/HE1_qZhPj1I.

Huh.  Shows how closely I follow non-SpiderMonkey development any more!

Comment 48

[Kershaw Chang [:kershaw]]

Attachment: Part1: Support keep-alive, r=Waldo

Summary:
 - Address review comments
 - Carry reviewer's name

Comment 49

[Kershaw Chang [:kershaw]]

Attachment: Part2: Modify failed tests, r=Waldo

Comment 50

[Kershaw Chang [:kershaw]]

Attachment: Part3: Close all connections at the end, r=Waldo

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e22c0308a7e
Part1: Support keep-alive, r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d46046a7367
Part2: Modify tests that can not pass, r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/1431c80b02ef
Part3: Prune all connection at the end of test, r=Waldo

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This may have caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=98065165&repo=mozilla-inbound to start happening frequently on Windows

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4f45c9de5a9e

Comment 53

[Kershaw Chang [:kershaw]]

(In reply to Wes Kocher (:KWierso) from comment #52)
> This may have caused failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=98065165&repo=mozilla-
> inbound to start happening frequently on Windows
> 
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4f45c9de5a9e

Got it. I'll take a look.
Thanks.

Comment 54

[Honza Bambas (:mayhemer)]

It would be so nice to fix this. I would like to have a test for bug 1522093 but it can't be done w/o persistent connections. It's a shame all our test infra runs on HTTP/1.0!

Comment 55

[Kershaw Chang [:kershaw]]

(In reply to Honza Bambas (:mayhemer) from comment #54)

It would be so nice to fix this. I would like to have a test for bug 1522093 but it can't be done w/o persistent connections. It's a shame all our test infra runs on HTTP/1.0!

Last time this ended up with some unexpected test failures on try. I really have no idea how to fix those tests.
Anyway, I'll try to land this again.

Comment 56

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

You could make keep-alive disabled by default, so we don't have to deal with the old tests.

Comment 57

[Kershaw Chang [:kershaw]]

Not working on this one.

Comment 58

[Valentin Gosu [:valentin] {{ PTO 2 - 12 April }}]

We now have NodeServer.sys.mjs as a full featured replacement for httpd.js
So I'm not sure if we still want to land this, or we can just close it.