462864 - Expose request body as a stream, state-machine refactoring

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Partly-tested patch

The server is intended to be usable as an XPCOM component, so anything JS-specific like using httpd.js-internal data structures in the API (LineData here) is verboten.  Also, the state machine in request processing is currently a bit weird, because it pushes common functionality too low, as demonstrated by the weirdness around body processing -- change that to simplify and clarify the code.

I had some time on my flights out to Mountain View yesterday to noodle on this while working toward allowing async response processing, and the following patch should address these problems.  It runs the netwerk "check" tests fine, but I haven't run anything else (particularly Mochitest) to see whether it still has any bugs -- need to run the usual stuff before getting any reviews.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Ready to go

This is basically the same patch with a few added naming tweaks and a couple trivial code tweaks.  xpcshell and Mochitests are doing fine with this patch.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Actually, add a test for the Content-Length leading-zero bug, too

I realized I'd forgotten a test; the nice thing is that writing the test exposed a hang -- I was reading the "excess" data based on the octal value and then not hitting an == comparison due to |this._contentLength| dropping below zero.  I'm pretty sure that's the only substantive bug this fixed, so I don't think any other tests need to be written.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 346487 [details] [diff] [review]
Actually, add a test for the Content-Length leading-zero bug, too

This patch is actually addressing your comments to my patch, more or less, right?
1. You mentioned we should return 201 in response to PUT. We should, but when the new resource had been created, not when modified (the we have to return 200 or 204). It is hard to decide if the resource had been created or not, at least with my limited knowledge of httpd.js
2. Why did you change those two sjs? was the previous code wrong? There is no change (as I see) in code sending the response body.

I should change the test for PUT/GET to the new runHttpTests way. I didn't know there is such framework.

Comment 4

[Jeff Walden [:Waldo]]

Well, this was primarily the result of some thinking I did while hiking after seeing the weird way |this._state| had to be handled to accommodate processing the body of requests.  Clearly, the fallthrough from state to state shouldn't be duplicated in every _process* function.  The fixes I made that happened to address things I mentioned in response to your patch were just typo-level things that I fixed along the way -- mostly just formatting concerns with no semantic impact.  In particular, changing PUT/GET wouldn't be incidental because it raises spec compliance concerns that would require some thought to implement and to review for correctness; I don't really know all the detail of PUT/GET.

As for the numbers:

1) Shouldn't |var present = path in this._putOverrides;| or something work for determining prior existence?
2) The change is in how the body is exposed -- you'd exposed it as a LineData object (a structure intended to be internal to httpd.js) that's not usable if the server is being run as an XPCOM component, and I changed it to an XPCOM type so that anyone (including compiled code, for example) could use it.

Yeah, I needed runHttpTests after realizing that the original approach required too much plumbing and was next to impossible to read, understand, and modify due to the necessary separations of continuous control flow.  I seem to recall the old way also discouraged me from making changes, because writing the necessary tests was such a pain to do -- something had to give.  :-)

Comment 5

[Jeff Walden [:Waldo]]

Landed, with the Content-Length change tested as well.  Whatever nits I didn't address in my comments on your patch we can move to a separate bug; my goals in this bug have been met.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #4)
> In particular, changing PUT/GET wouldn't be incidental
> because it raises spec compliance concerns that would require some thought to
> implement and to review for correctness; I don't really know all the detail of
> PUT/GET.

I'll file a new bug and fix the concerns. But, btw, Jonas Sicking found out this breaks ability to let sjs handle PUT and GET (bug 443610 comment 14). Question is if his workaround is good enough or we should consider prioritization of sjs before put data. Problem is that I already have a test that uses PUT to temporarily override an sjs to simulate an on-demand redirect under the same URL. PUT and DELETE seemed to me a naturalist way to be able to modify data on the server. However, we actually don't need properly implemented PUT and DELETE, we just need something that drives remotely state on the server, to modify a response not only by modifying a request. Actually, uploading a modified sjs or tell an sjs to set a particular state would be more appropriate and flexible (we can modify headers, response status too) and wouldn't collide with anything existing.

Comment 7

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

I really don't think we want to remove the ability for sjs files to handle handle PUT and DELETE. In my case it was easy to work around as the actual HTTP method I was using didn't matter for those two methods, however that might not always be the case. For example if we want to test an implementation of AtomPub or some such.

In most server APIs (such as CGI) the HTTP method is just forwarded to script handling the resource.

A better solution is IMHO to allow sjs files to keep state between invocations. This means that you could implement the PUT/DELETE functionality that you've done inside of an sjs file instead.

Comment 8

[Jeff Walden [:Waldo]]

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

Comment 9

[Jeff Walden [:Waldo]]

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