485255 - Add ability to add prefix handler to httpd.js

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mook (songbird dead account)]

Attachment: add new method on idl, tests, make usable as jsm

attached patch adds a registerPrefixHandler method to httpd.js (like registerPathHandler, but needs a directory) so I can deal with a whole subdirectory at a time instead of a file.

also randomly makes httpd.js usable from Components.utils.import because I can't get at HttpError otherwise!

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 369382 [details] [diff] [review]
add new method on idl, tests, make usable as jsm

Sorry for the very long delay here...r- for the things below, plus the good likelihood that this has bitrotted.


>diff --git a/netwerk/test/httpserver/httpd.js b/netwerk/test/httpserver/httpd.js

>+const EXPORTED_SYMBOLS = ["server", "nsHttpServer", "HttpError"];

The server function definitely shouldn't be exported; it's for manual testing and nothing more.  HttpError as an interface is unsatisfactory at the moment.  I don't know what the right solution is yet, but it's not the current system.

Something I also want to do eventually is kill the ns prefix in httpd.js, which this would make slightly (slightly) more painful to do.  Just worth keeping in mind, not something to hold this up.


>@@ -2031,7 +2050,26 @@
>           this._overridePaths[path](metadata, response);
>         }
>         else
>-          this._handleDefault(metadata, response);
>+        {
>+          let longestPrefix = "";

General style comment: httpd.js uses standard JS, no extra extensions beyond getters/setters (which are ES5 now anyway), so |let| is out; |var| is the new |let|.  An "exception" are the array extras because they're in ES5 now and can be trivially backported for engines that don't support ES5 (which kind of makes it not really an exception, sort of), but I don't think we've used them yet.


>+          for (let prefix in this._overridePrefixes)
>+          {
>+            if (prefix.length > longestPrefix.length &&
>+                path.substr(0, prefix.length) == prefix)
>+            {
>+              longestPrefix = prefix;
>+            }
>+          }
>+          if (longestPrefix.length > 0)
>+          {
>+            dumpn("calling prefix override for " + longestPrefix);
>+            this._overridePrefixes[longestPrefix](metadata, response);
>+          }
>+          else
>+          {
>+            this._handleDefault(metadata, response);
>+          }
>+        }

I think it's about reached the time to make all these overrides, static registrations, directory handlers, and such be handled on a path-component-by-path-component basis, so for /foo/bar/baz/ we'd run through the transformations that apply to /, then /foo/, then /foo/bar/, and so on.  This bug is arguably not the place to address that, except that that for-loop over all prefix handlers there hurts, and it would hurt the vast majority of requests made of the server.  I don't think I can accept that algorithmic-complexity overhead.

Comment 2

[Jeff Walden [:Waldo]]

(Oh, plus for the moment const is acceptable, on the topic of permitted extensions.)

Comment 3

[Gregory Szorc [:gps]]

Attachment: Path prefix handlers, v2

I have a need for this functionality and have updated the patch. I fixed bitrot and addressed comments from prior feedback. I also made some minor style changes to the unit test. I still tried to preserve the existing style conventions of this code. Try at https://tbpl.mozilla.org/?tree=Try&rev=eb236f017dae.

Comment 4

[Mozilla RelEng Bot]

Try run for eb236f017dae is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=eb236f017dae
Results (out of 137 total builds):
    success: 122
    warnings: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-eb236f017dae

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 618808 [details] [diff] [review]
Path prefix handlers, v2

Attempting to find another reviewer since :Waldo is busy.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e840ffbba8d1

Thanks for the review!

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e840ffbba8d1

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 618808 [details] [diff] [review]
Path prefix handlers, v2

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

Aargh!  I had review comments written on this that I could have sworn I submitted.  :-(  Maybe the request disappeared from my queue at just the wrong time that I didn't notice the non-submission.

How entrenched is this?  I would rather we backed this out and integrated prefix handling with the per-path-component behavior storage we have for other parts of the server API.

::: netwerk/test/httpserver/httpd.js
@@ +2296,5 @@
> +                path.substr(0, prefix.length) == prefix)
> +            {
> +              longestPrefix = prefix;
> +            }
> +          }

This still contemplates a loop over all prefix overrides for every request, which I noted in the last paragraph of comment 1 wasn't acceptable.

::: netwerk/test/httpserver/test/test_registerprefix.js
@@ +1,1 @@
> +/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

JavaScript, I believe.

Comment 9

[Gregory Szorc [:gps]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #8)
> How entrenched is this?

Honestly, not very. But I would like the behavior soon, as I am writing test HTTP servers that need to intercept all paths and the least hacky way to do this is through prefix handlers (currently, we actually replace server._defaultHandler).

> I would rather we backed this out and integrated
> prefix handling with the per-path-component behavior storage we have for
> other parts of the server API.

Can you elaborate? I think you are advocating for the prefix match to terminate on path boundaries (/). If so, I am opposed to this. Well, I'm at least opposed to it being the only option.

There are RESTful services that may need to take over paths beyond one / delimiter. Forcing them to register multiple prefix handlers seems like overkill. I agree single-level is likely the most used. But, there may be multi-level users that need to be supported. I also like the "backdoor" where you can define "/" as a prefix handler and control the entire URI space. This can be very useful (such as ensuring a client doesn't make excessive requests).

Put another way, if a consumer wishes to perform his or her own routing and dispatching, let's let them.

> ::: netwerk/test/httpserver/httpd.js
> @@ +2296,5 @@
> > +                path.substr(0, prefix.length) == prefix)
> > +            {
> > +              longestPrefix = prefix;
> > +            }
> > +          }
> 
> This still contemplates a loop over all prefix overrides for every request,
> which I noted in the last paragraph of comment 1 wasn't acceptable.

This is for a testing-only server, not a piece of production infrastructure where speed is critical. And, if there are no path prefixes registered there is effectively no penalty. I don't see a concern.

Comment 10

[Jeff Walden [:Waldo]]

(In reply to Gregory Szorc [:gps] from comment #9)
> Can you elaborate? I think you are advocating for the prefix match to
> terminate on path boundaries (/). If so, I am opposed to this. Well, I'm at
> least opposed to it being the only option.

This is not what I'm advocating for, so I think we aren't at fundamental odds here.  :-)  Basically, I think we should unify the approach taken for looking up file-path mappings with the code used to look up custom path handlers, prefix handlers, and the like.  That is, for the path /path/to/file, we'd look up information for the "/path/to/" component.  If we find a custom override we hand off to that, otherwise if we find a prefix handler we hand off to that, otherwise if we find a directory mapping we hand off to that, and so on.  If all these fail, we search for info for the "/path/" component, doing the same steps.  Then we try for "/".  If all fails we 404, or whatever.  Right now these steps are run only for access to files in ServerHandler.prototype._getFileForPath.  It shouldn't be too difficult to generalize that to handle info for the other forms of request interception/handling that are in httpd.js.

> This is for a testing-only server, not a piece of production infrastructure
> where speed is critical. And, if there are no path prefixes registered there
> is effectively no penalty. I don't see a concern.

We use it only for testing, but it's been designed to a higher standard pretty much from the start.  As for speed, it does matter.  We run mochitests, reftests, and probably other tests I don't even know about using httpd.js, and over the course of an entire mochitest or reftest run, these sorts of costs begin to add up.  I'm very hesitant to accept a loop like this in a place where, with some refactoring of algorithms, we should be able to get constant-time (backed by hashes) behavior.

Comment 11

[Gregory Szorc [:gps]]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #10)
> (In reply to Gregory Szorc [:gps] from comment #9)
> > Can you elaborate? I think you are advocating for the prefix match to
> > terminate on path boundaries (/). If so, I am opposed to this. Well, I'm at
> > least opposed to it being the only option.
> 
> This is not what I'm advocating for, so I think we aren't at fundamental
> odds here.  :-)

Whew!

> Basically, I think we should unify the approach taken for
> looking up file-path mappings with the code used to look up custom path
> handlers, prefix handlers, and the like.  That is, for the path
> /path/to/file, we'd look up information for the "/path/to/" component.  If
> we find a custom override we hand off to that, otherwise if we find a prefix
> handler we hand off to that, otherwise if we find a directory mapping we
> hand off to that, and so on.  If all these fail, we search for info for the
> "/path/" component, doing the same steps.  Then we try for "/".  If all
> fails we 404, or whatever.  Right now these steps are run only for access to
> files in ServerHandler.prototype._getFileForPath.  It shouldn't be too
> difficult to generalize that to handle info for the other forms of request
> interception/handling that are in httpd.js.

Yeah, that makes sense. And, I think it is a better solution.

So, I'd prefer to not back out this feature because I plan on using it soon. However, I'll be using whole sub-path prefix handlers and I won't be impacted by behavioral changes (if there are any) if the routing implementation is changed under the hood.

Perhaps this could be the realm of a follow-up bug?

> 
> > This is for a testing-only server, not a piece of production infrastructure
> > where speed is critical. And, if there are no path prefixes registered there
> > is effectively no penalty. I don't see a concern.
> 
> We use it only for testing, but it's been designed to a higher standard
> pretty much from the start.  As for speed, it does matter.  We run
> mochitests, reftests, and probably other tests I don't even know about using
> httpd.js, and over the course of an entire mochitest or reftest run, these
> sorts of costs begin to add up.

I highly doubt this will be measurable unless you are introducing dozens of prefix routes on the server. As it stands, we have 0 tests even using this feature. So, the penalty is effectively "iterate over an empty object." If you can measure that against the overhead of a string-based protocol like HTTP, I would be surprised.