Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Allow empty Reason-Phrase #131

Merged
merged 2 commits into from Mar 16, 2014

Conversation

pyrtsa
Copy link
Contributor

@pyrtsa pyrtsa commented Mar 10, 2014

With the current code in master, the condition in https://github.com/http-kit/http-kit/blob/master/src/java/org/httpkit/client/Decoder.java#L55-L58 still fails to work for a correctly behaving HTTP server that just happens to omit the Reason-Phrase returning a Status-Line with a space following the Status-Code, e.g. "HTTP/1.1 200 ".

This pull request fixes HttpUtils/findEndOfString to handle this case correctly, and adds a set of unit tests that check the behavior of org.httpkit.client.Decoder.

The Decoder class unexpectedly fails to decode a response starting with

    "HTTP/1.1 200 \r\n"

while the following initial line is accepted even if, strictly speaking,
against the RFC 2616:

    "HTTP/1.1 200\r\n"

(I think both should be accepted by HTTP Kit.)
Use an `offset` argument with `HttpUtils/findEndOfString` to make it behave
consistently with the rest of the `find*` string methods.
@pyrtsa
Copy link
Contributor Author

pyrtsa commented Mar 11, 2014

Once more, if it wasn't clear from the description, let me point out that the RFC 2616 actually requires the space after the Status-Code, even if the Reason-Phrase is an empty string (which is also allowed):

Status-Line = HTTP-Version SP Status-Code SP Reason-Phrase CRLF
Reason-Phrase  = *<TEXT, excluding CR, LF>

@shenfeng
Copy link
Member

Thanks! Will look into this once time is allowed. Now in a busy project with a strict deadline.

@pyrtsa
Copy link
Contributor Author

pyrtsa commented Mar 13, 2014

FYI, this bug is getting more important as Facebook is apparently rolling out its new server versions that omit the Reason-Phrase in all responses:

$ curl -I https://www.facebook.com
HTTP/1.1 200 
Cache-Control: private, no-cache, no-store, must-revalidate
...

shenfeng added a commit that referenced this pull request Mar 16, 2014
@shenfeng shenfeng merged commit 19fa0fe into http-kit:master Mar 16, 2014
@shenfeng
Copy link
Member

Sorry for respond late.
The code looks pretty good, I like it. Thanks.

@mpenet
Copy link
Contributor

mpenet commented Mar 18, 2014

Is there a release planned relatively quickly? I am relying on http-kit to interact with the facebook api quite heavily and this hit us badly.

Thanks

@shenfeng
Copy link
Member

Just push

[http-kit "2.1.18"]

to clojars.

With other few bug fixes.

@mpenet
Copy link
Contributor

mpenet commented Mar 20, 2014

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants