Comments (18)
Confirmed.
It's annoying: while it's obviously a bug, fixing it breaks a number of assumptions that http-parser users may have made about the order of events or when it's safe to call the parser (evidenced by the number of tests the fix breaks).
Rock, meet hard place.
EDIT: I've added a failing test in bnoordhuis/http-parser@e3028fe.
from http-parser.
I've hacked a workaround in AndreLouisCaron/httpxx@608af01, but it's really ugly. I have to manually fix off-by-one errors because of this bug, and the count is unreliable until the on_message_complete()
callback is called so that I can fix it.
If someone has a clear idea on how to fix this issue without breaking everything, I might be able to put in some time to fix it.
from http-parser.
Yeah, this is tricky to fix because a single byte can trigger multiple callbacks, and if a callback other than the last results in the parser being paused, we need a way to track that these addition callbacks need to be executed. We're doing this now by not consuming the byte until we're done with all of the callbacks that it should trigger. For callbacks where we do not re-execute the same byte, this isn't an issue, of course.
We could fix this to return that all bytes were consumed (e.g. by stashing the last byte in http_parser
and using it when continuing execution), but we'd be breaking the assumption of users of the http_parser_execute()
API that if all bytes are consumed, there is no reason to call http_parser_execute()
again until there is more data to parse (or an EOF has been received). We'd also need a way to poke the parser to consume this stashed byte -- calling http_parser_execute(NULL, 0)
isn't going to cut it, as that would look eerily like an EOF to the un-trained eye. Some options here include adding a new http_parser_execute_buffered()
API, or having http_parser_pause(0)
kick off execution.
Mercifully, these changes in execution behavior might okay-ish because these assumptions about return value/byte consumption only break if you're using http_parser_pause()
, and AFAIK there are a limited number of users of this function (possibly only us and AndreLouisCaron/httpxx).
Any other ideas @bnoordhuis, @ry or @clifffrey?
from http-parser.
I'll go out on a limb here, but it seems to me that we're approaching the problem from the wrong angle. Rather than trying to fix the thorny issue of marking the last byte, wouldn't it be easier to play on the pausing semantics? I don't really care if more callbacks are invoked after I pause the parser (e.g. on_message_complete()
for an empty message), so long as it pauses "soon".
Pausing is currently implemented by using a special error code, which happens to be temporary (the client can reset it by calling http_parser_pause(&parser, 0);
). The real issue here it that the parser does not distinguish between HPE_PAUSED
and real error codes. A more flexible approach to pausing would prevent the parser from continuing after a change of state (e.g. from "parsing headers" to "parsing body").
Any ideas on this?
from http-parser.
Is node (or any other library) relying on http-parser not executing on_message_complete
after pausing from inside on_headers_complete
?
If not, I think the easiest 'fix' would be to implement pause as "don't consume any more bytes (but call whatever callbacks are necessary)" instead of "stop doing anything". That was what I thought it did when I read the prototype of http_parser_pause
anyway.
from http-parser.
I don't really care if more callbacks are invoked after I pause the parser (e.g. on_message_complete() for an empty message), so long as it pauses "soon".
I think having a immediate, non-advisory pause is pretty useful, particularly as evidenced by the Node community's continued struggles to deal with its absence: the fact that ServerRequest.pause()
doesn't actually pause bites people frequently, and is a work-around that looks likely to land in the latest release (by buffering in the application) is both highly desired and looked at with skepticism because of what it has to do to work around the fact that http_parser
couldn't natively pause (they don't know about http_parser_pause()
yet, it seems).
That said, there's certainly a trade-off between implementation complexity and value, but I don't think we've crossed that threshold.
The real issue here it that the parser does not distinguish between HPE_PAUSED and real error codes.
Is your expectation that calling http_parser_execute()
should implicitly un-pause the parser? If so, I'm not sure I follow how that's related to reporting the correct number of bytes consumed.
from http-parser.
Is node (or any other library) relying on http-parser not executing on_message_complete after pausing from inside on_headers_complete?
Not that I know of. AFAIK there are very few users of http_parser_pause()
.
If not, I think the easiest 'fix' would be to implement pause as "don't consume any more bytes (but call whatever callbacks are necessary)" instead of "stop doing anything".
Yeah, that's certainly an option.
I do think it's worth spending some amount of effort to get the API to work with the more rigid semantics, though, as this provides a more powerful API. I'll spend some on that this weekend and see how far I get.
from http-parser.
Couldn't we store the pause state somewhere explicitly? It would be more
work to code up, but if we had different parser states for these things,
even though they were at the same byte offset, then the parser could know
which callbacks have been run already.
I think that pause needs to be absolute, it would be very surprising to a
new user of the API to see a callback called after the stream was paused.
On Mon, Feb 27, 2012 at 11:50 AM, Peter Griess <
[email protected]
wrote:
Is node (or any other library) relying on http-parser not executing
on_message_complete after pausing from inside on_headers_complete?Not that I know of. AFAIK there are very few users of
http_parser_pause()
.If not, I think the easiest 'fix' would be to implement pause as "don't
consume any more bytes (but call whatever callbacks are necessary)" instead
of "stop doing anything".Yeah, that's certainly an option.
I do think it's worth spending some amount of effort to get the API to
work with the more rigid semantics, though, as this provides a more
powerful API. I'll spend some on that this weekend and see how far I get.
Reply to this email directly or view it on GitHub:
#97 (comment)
from http-parser.
If not, I think the easiest 'fix' would be to implement pause as "don't consume any more bytes (but call whatever callbacks are necessary)" instead of "stop doing anything".
Yeah, that's certainly an option.
Sorry if I wasn't clear when I said I wanted the parser to stop "soon". This is what I had in mind.
from http-parser.
I think the easiest 'fix' would be to implement pause as "don't consume any more bytes (but call whatever callbacks are necessary)" instead of "stop doing anything".
I've worked around the issue to implement exactly this in AndreLouisCaron/httpxx@fbd0422. It only works for processing the last byte after the headers, but it fixes the only instance of this problem I've encountered so far (somehow, pausing in on_message_complete()
correctly reports all bytes as consumed).
I would still prefer this was fixed in http-parser
for all (potential) cases, but if there is a serious design problem in implementing a real fix, I can live with this bug as long as the exact behavior of http_parser_pause()
is documented so that no one gets "bitten" by this problem.
from http-parser.
I would still prefer this was fixed in http-parser for all (potential) cases, but if there is a serious design problem in implementing a real fix, I can live with this bug
I think everyone agrees that it should be fixed on our side, the question is how?
from http-parser.
I looked at bnoordhuis's new test case and it prevents pausing in on_headers_complete
from invoking on_message_complete
. I don' t think this can ever be satisfied if we fix this issue since empty requests absolutely need to call both callbacks before marking the last byte as consumed.
I pushed a sample fix that just addresses the specific case of letting on_message_complete
be called when the parser is paused from on_headers_complete
. It breaks current tests for pausing, which validate that absolutely no more callbacks should be called after a pause. All other tests still pass, but don't use this since I think this is somewhat of a hack, and I'm not sure what the behavior is for pausing in other callbacks (e.g. on_url()
).
I think everyone agrees that it should be fixed on our side, the question is how?
First, we need to define the semantics of pausing. Should pausing be allowed all callbacks, or should it be limited to specific cases? AFAICT, the only interesting places to call this are in on_headers_complete
for proxying purposes and in on_message_complete
for HTTP/1.1 pipelined requests.
Also, in what (other) cases is this a problem? So far, the only problematic instance is for on_headers_complete()
, which must potentially call on_message_complete
when the request has no body. Are there any other situations that can call more than one callback for a single byte?
Since it already works for the on_message_complete
callback and it's a question of a single byte in on_headers_complete
and other cases don't seem so interesting, can we just hack in a solution for the on_headers_complete
case and document http_parser_pause
as only supported in those two callbacks?
from http-parser.
I feel like I'm missing something.. why is it that "... empty requests
absolutely need to call both callbacks before marking the last byte as
consumed"? Couldn't they consume the last byte, but leave the parser in a
state of s_paused_before_message_complete, and have that state just call
the callback and then do "goto reexecute_byte;"? I'm probably missing
something though, so please forgive me if this idea can't work.
On Tue, Feb 28, 2012 at 9:21 PM, Andr Caron <
[email protected]
wrote:
I looked at bnoordhuis's new test case and
it prevents pausing inon_headers_complete
from invoking
on_message_complete
. I don' t think this can ever be satisfied if we fix
this issue since empty requests absolutely need to call both callbacks
before marking the last byte as consumed.I pushed a sample fix that just addresses the specific case of letting
on_message_complete
be called when the parser is paused from
on_headers_complete
. It breaks current tests for pausing, which validate
that absolutely no more callbacks should be called after a pause. All
other tests still pass, but don't use this since I think this is somewhat
of a hack, and I'm not sure what the behavior is for pausing in other
callbacks (e.g.on_url()
).I think everyone agrees that it should be fixed on our side, the
question is how?First, we need to define the semantics of pausing. Should pausing be
allowed all callbacks, or should it be limited to specific cases? AFAICT,
the only interesting places to call this are inon_headers_complete
for
proxying purposes and inon_message_complete
for HTTP/1.1 pipelined
requests.Also, in what (other) cases is this a problem? So far, the only
problematic instance is foron_headers_complete()
, which must potentially
callon_message_complete
when the request has no body.
Reply to this email directly or view it on GitHub:
#97 (comment)
from http-parser.
After further reading of the code, it seems to me that the only risky bits are parser states that use goto reexecute_byte;
and execute callbacks. I only found only two states that do this:
s_body_identity
; ands_headers_almost_done
.
FWIW, the latter is our problematic case and the former already has somewhat of a hack:
/* Mimic CALLBACK_DATA_NOADVANCE() but with one extra byte.
*
* The alternative to doing this is to wait for the next byte to
* trigger the data callback, just as in every other case. The
* problem with this is that this makes it difficult for the test
* harness to distinguish between complete-on-EOF and
* complete-on-length. It's not clear that this distinction is
* important for applications, but let's keep it for now.
*/
CALLBACK_DATA_(body, p - body_mark + 1, p - data);
goto reexecute_byte;
from http-parser.
Couldn't they consume the last byte, but leave the parser in a state of s_paused_before_message_complete, and have that state just call the callback and then do "goto reexecute_byte;"?
Hadn't thought of introducing anoher state! However, I don't think this will work, since the byte will no longer available on the next call to http_parser_execute
(the client will have considered it already consumed by the parser). Also, that would mean that on an empty request, the client would have to call http_parser_execute
with 0 bytes to "complete" the message even though all the message data has been processed, and I believe this breaks one of the key invariants of http-parser
. See pgriess's earlier comment.
from http-parser.
Yeah, @AndreLouisCaron is right that the particular devil here is the extra byte that you need to stash: where to stash it, and how to kick off parsing with it. I don't think we need to add any more parser states to accomplish this, as there is already a state for every place where we might have paused (i.e. after every callback). We just need to tweak these states to look at the stashed byte.
I'm starting to think that renaming http_parser_pause()
to http_parser_unpause_and_execute()
(or something) is the right way to go. I can't think of a meaningful situation where one would un-pause the parser without wanting to then immediately process any data that remained. The major benefit of this API name change would be that it's obvious that the parser going to start executing without being given any additional data.
from http-parser.
@indutny @bnoordhuis ... any reason to keep this open?
from http-parser.
I'm about to open a new issue. I'd like my C++ wrapper to operate the parser in two stages, first to process the message header as a complete unit, and then to process the body. Returning 1 from on_headers_complete
is not the answer because you can't resume parsing of the body. So I'm looking through the outstanding issues to see if anyone else seeks similar functionality and I came across this issue.
I'm going to try using the pause feature to implement two stage parsing. Was the bug described fixed? Am I going to encounter a problem with "that last byte?" Is there another way to implement the functionality I desire?
from http-parser.
Related Issues (20)
- After upgrade it stoped working HOT 10
- Handle URLs with a colon after host but no port HOT 8
- 2.9.3 breaks ABI compatibility with 2.9.2 with no corresponding SONAME change HOT 6
- An http-parser update broke etherpad HOT 4
- Suggestion for making F_SKIPBODY more useful
- armv7hl: Assertion `sizeof(http_parser) == 4 + 4 + 8 + 2 + 2 + 4 + sizeof(void *)' failed HOT 5
- Documentation update request: (0 if no Content-Length header) is incorrect HOT 2
- Fix -Wsign-compare warning
- Chunked may not be final in transfer encoding list HOT 8
- Request with Transfer-Encoding: chunked and Content-Length is valid per RFC, but rejected with HPE_UNEXPECTED_CONTENT_LENGTH HOT 18
- Parse Error: Invalid header value char HOT 2
- Maintainer notice
- Have "Invalid header value char" error message more explicit HOT 1
- Test for the size of struct http_parser fails on 32 bit systems where there is padding/alignment for void* HOT 6
- libhttp-parser-ext.bundle, 5): no suitable image found HOT 4
- if I put two http request buf, function http_context_parser will crash,why???
- Examples HOT 6
- Rename primary branch to main HOT 2
- Incremental builds with no changes still trigger refactoring
- https://github.com/nodejs/help.wiki.git
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from http-parser.