Generic method #145

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
6 participants
@cmr
Contributor

cmr commented Dec 16, 2012

Should close up #92 and pave the way for joyent/node#3192. Currently depends on #144 or #106 being merged, because I wasn't paying attention and I don't know how to rebase it. Tests pass, but I'm still not confident in the patch quality. Review desired.

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Dec 16, 2012

Indent error.

Indent error.

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Dec 16, 2012

This can't conceivably be a valid request, can it?

This can't conceivably be a valid request, can it?

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 16, 2012

Owner

No, it can't be. Back to the drawing board.

Owner

cmr replied Dec 16, 2012

No, it can't be. Back to the drawing board.

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 16, 2012

Owner

It's not because it's an invalid method, though. I need to hit the spec.

Owner

cmr replied Dec 16, 2012

It's not because it's an invalid method, though. I need to hit the spec.

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 17, 2012

Owner

It's invalid because "world" is not a valid URI, but also because of the lack of CRLF. Given lack of HTTP-Version would make it an 0.9 or 1.0 Simple-Request, if there were a CRLF and a valid URI. How should this be handled?

Owner

cmr replied Dec 17, 2012

It's invalid because "world" is not a valid URI, but also because of the lack of CRLF. Given lack of HTTP-Version would make it an 0.9 or 1.0 Simple-Request, if there were a CRLF and a valid URI. How should this be handled?

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Dec 17, 2012

Not sure. Gut feeling says it should fail because of the missing CRLF because you could argue that "world" is an unqualified domain name (as in CONNECT world - though that's a bad example because CONNECT requires a host:port combo.)

Maybe stick with HPE_INVALID_PATH for now and wait for the bug reports to come in? :-)

Not sure. Gut feeling says it should fail because of the missing CRLF because you could argue that "world" is an unqualified domain name (as in CONNECT world - though that's a bad example because CONNECT requires a host:port combo.)

Maybe stick with HPE_INVALID_PATH for now and wait for the bug reports to come in? :-)

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 17, 2012

Owner

Sounds good to me, I'll poke at it later today.

Owner

cmr replied Dec 17, 2012

Sounds good to me, I'll poke at it later today.

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 19, 2012

Contributor

I'm having some trouble fixing this one. The parser runs out of data before it actually finds anything invalid. At http_parser.c:1819, the default behaviour seems to be to assume that the request was valid. Mind taking a peek?

Contributor

cmr commented Dec 19, 2012

I'm having some trouble fixing this one. The parser runs out of data before it actually finds anything invalid. At http_parser.c:1819, the default behaviour seems to be to assume that the request was valid. Mind taking a peek?

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Contributor

I think adding a bit to http_parser indicating whether, given the current state, the request/response is valid if parsing were to stop immediately is a good solution. Any thoughts before I code it up later today?

Contributor

cmr commented Dec 20, 2012

I think adding a bit to http_parser indicating whether, given the current state, the request/response is valid if parsing were to stop immediately is a good solution. Any thoughts before I code it up later today?

http_parser.c
+ if (ch == 'E') {
+ parser->method = HTTP_HEAD;
+ } else {
+ parser->type = HTTP_REQUEST;
}

This comment has been minimized.

Show comment Hide comment
@kolbyjack

kolbyjack Dec 20, 2012

Contributor

I think this should always set parser->type to HTTP_REQUEST, and set parser->method to HTTP_HEAD or HTTP_GENERIC depending on ch.

@kolbyjack

kolbyjack Dec 20, 2012

Contributor

I think this should always set parser->type to HTTP_REQUEST, and set parser->method to HTTP_HEAD or HTTP_GENERIC depending on ch.

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Contributor

Looking back the logic I wrote doesn't even make sense. "HTTP" is a valid extension method, so setting the type to RESPONSE before encountering a / doesn't make sense.

@cmr

cmr Dec 20, 2012

Contributor

Looking back the logic I wrote doesn't even make sense. "HTTP" is a valid extension method, so setting the type to RESPONSE before encountering a / doesn't make sense.

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Contributor

Fixed up in ff4d643

@cmr

cmr Dec 20, 2012

Contributor

Fixed up in ff4d643

http_parser.c
@@ -918,7 +921,8 @@ size_t http_parser_execute (http_parser *parser,
}
matcher = method_strings[parser->method];
- if (ch == ' ' && matcher[parser->index] == '\0') {
+ if (ch == ' ' && (matcher[parser->index] == '\0' || parser->method == HTTP_GENERIC)) {
+ CALLBACK_DATA(method);
parser->state = s_req_spaces_before_url;

This comment has been minimized.

Show comment Hide comment
@kolbyjack

kolbyjack Dec 20, 2012

Contributor

If parser->method is HTTP_GENERIC, you need to avoid checking matcher[parser->index] or REALLYLONGMETHODNAME will have you walking off the end of the array.

@kolbyjack

kolbyjack Dec 20, 2012

Contributor

If parser->method is HTTP_GENERIC, you need to avoid checking matcher[parser->index] or REALLYLONGMETHODNAME will have you walking off the end of the array.

cmr added some commits Dec 20, 2012

Add generic method test
Reverted a simple test to succeed even though the result is invalid, because
there are other more important things that need doing in this patch (like
getting it to actually work)
@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Owner

What should this be? Not exactly sure what the semantics of it are (it doesn't help that I'm not all that familiar with the http spec)

Owner

cmr commented on test.c in 7d7852d Dec 20, 2012

What should this be? Not exactly sure what the semantics of it are (it doesn't help that I'm not all that familiar with the http spec)

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Owner

To be fixed later

Owner

cmr commented on test.c in 7d7852d Dec 20, 2012

To be fixed later

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Owner

What should be done here? Isn't there going to be a relatively significant performance hit having to go though the loop for detecting every supported method?

Owner

cmr commented on http_parser.c in ff4d643 Dec 20, 2012

What should be done here? Isn't there going to be a relatively significant performance hit having to go though the loop for detecting every supported method?

@kolbyjack

This comment has been minimized.

Show comment Hide comment
@kolbyjack

kolbyjack Dec 20, 2012

Contributor

I think you need to goto reexecute_byte when switching to a request here in case ch is a space

Contributor

kolbyjack commented on http_parser.c in ff4d643 Dec 20, 2012

I think you need to goto reexecute_byte when switching to a request here in case ch is a space

This comment has been minimized.

Show comment Hide comment
@kolbyjack

kolbyjack Dec 20, 2012

Contributor

Since it's walking through the full HTTP now, would it be better to deal with s_req_or_resp as a single state kind of like the request methods? Use a string "HTTP" and parser->index?

Contributor

kolbyjack replied Dec 20, 2012

Since it's walking through the full HTTP now, would it be better to deal with s_req_or_resp as a single state kind of like the request methods? Use a string "HTTP" and parser->index?

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Contributor

That would simplify the code a lot, great suggestion (thanks for all the help!)

Contributor

cmr replied Dec 20, 2012

That would simplify the code a lot, great suggestion (thanks for all the help!)

@kolbyjack

This comment has been minimized.

Show comment Hide comment
@kolbyjack

kolbyjack Dec 20, 2012

Contributor

This will still fail on GENERICLONGMETHOD, since that 'L' isn't a space, it'll fall through to where it still indexes method_strings

Contributor

kolbyjack commented on http_parser.c in ff4d643 Dec 20, 2012

This will still fail on GENERICLONGMETHOD, since that 'L' isn't a space, it'll fall through to where it still indexes method_strings

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Dec 20, 2012

Contributor

Herp, I knew there was a reason I initially had a nested if.

Contributor

cmr commented on ff4d643 Dec 20, 2012

Herp, I knew there was a reason I initially had a nested if.

@udp udp referenced this pull request in udp/lacewing Jan 18, 2013

Open

Add support for the OPTIONS method #53

@udp

This comment has been minimized.

Show comment Hide comment
@udp

udp Aug 20, 2013

Contributor

Any update on accepting this? I really need support for custom methods but I'd rather not use a fork.

Contributor

udp commented Aug 20, 2013

Any update on accepting this? I really need support for custom methods but I'd rather not use a fork.

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Aug 20, 2013

Contributor

@udp The patch still doesn't quite work iirc. Want me to pick it back up?

Contributor

cmr commented Aug 20, 2013

@udp The patch still doesn't quite work iirc. Want me to pick it back up?

@udp

This comment has been minimized.

Show comment Hide comment
@udp

udp Aug 20, 2013

Contributor

That'd be great if you could. I don't mind doing some testing.

Contributor

udp commented Aug 20, 2013

That'd be great if you could. I don't mind doing some testing.

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Aug 20, 2013

Contributor

alright, I'll get to it later today.

On Tue, Aug 20, 2013 at 6:40 AM, James McLaughlin
notifications@github.comwrote:

That'd be great if you could. I don't mind doing some testing.


Reply to this email directly or view it on GitHubhttps://github.com/joyent/http-parser/pull/145#issuecomment-22936106
.

Contributor

cmr commented Aug 20, 2013

alright, I'll get to it later today.

On Tue, Aug 20, 2013 at 6:40 AM, James McLaughlin
notifications@github.comwrote:

That'd be great if you could. I don't mind doing some testing.


Reply to this email directly or view it on GitHubhttps://github.com/joyent/http-parser/pull/145#issuecomment-22936106
.

@mikedeboer

This comment has been minimized.

Show comment Hide comment
@mikedeboer

mikedeboer Jun 11, 2014

@cmr to achieve full-spec CalDAV support for our project, mikedeboer/jsDAV, this'd be awesome to have!

I'm guessing more and more ppl will stumble upon this sooner or later, but you'll be my hero to start with ;)

@cmr to achieve full-spec CalDAV support for our project, mikedeboer/jsDAV, this'd be awesome to have!

I'm guessing more and more ppl will stumble upon this sooner or later, but you'll be my hero to start with ;)

@cmr

This comment has been minimized.

Show comment Hide comment
@cmr

cmr Jun 11, 2014

Contributor

I'm no longer interested in working on this. Sorry!

Contributor

cmr commented Jun 11, 2014

I'm no longer interested in working on this. Sorry!

@cmr cmr closed this Jun 11, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment