Fix header fragmentation parsing (Fix #21)

[youknowone]

No description provided.

[youknowone]

I slightly changed the fragmented point and it now starts to be broken

[spaceone]

I think it was on purpose where the splitting was, especially the empty HTTP header.

[spaceone]

Another test which feeds byte-for-byte would also be nice.

[youknowone]

Thanks for the advice. I added a few more tests

[yohanboniface]

Not sure why you moved this check out of _maybe_call_on_header (so it's now duplicated).
Seems to me you could keep it in there, but just move the reset of _current_XXX out of if, something like:

def _maybe_call_on_header():
   if self._current_header_name is not None:
       # do the real call
   # do the reset

Thoughts?

[yohanboniface]

Not sure about the if/elif/else here.
Why not just calling _maybe_call_on_header if _current_header_name is None (which means we are starting a new header) ?

[youknowone]

updated

[yohanboniface]

I still don't get why we need to check _current_header_value here, given it's reset at each call to _maybe_call_on_header, but I'm certainly missing something :)

Seems to me this cover a case where both _current_header_name and _current_header_value would be set, but I fail to understand how this could happen, given they are reset all together when calling _maybe_call_on_header and any call to _on_header_field with an empty header name would call _maybe_call_on_header, and thus reset _current_header_value.

Can you elaborate a bit? :)

[youknowone]

One header field can trigger multiple _on_header_field call. (the else part)
I think it must be there unless you know better method to check it without _current_header_value.

[yohanboniface]

Unless I'm missing something, I understand that _on_header_field will be called (once or more) for the header name only, so it seems to me that the check on _current_header_value is useless.

Is there a test that fails if you remove it?

[youknowone]

I think that's the only part I fixed for the problem.
Yes, than the tests I added fail.

I am checking weather it is same header name or not by checking the header value. Once the value is fed, it must be a new header name.

[yohanboniface]

OK, now I think I get it :)

Here is the fix I suggest (from master):

diff --git a/httptools/parser/parser.pyx b/httptools/parser/parser.pyx
index 064e55c..b9b7175 100644
--- a/httptools/parser/parser.pyx
+++ b/httptools/parser/parser.pyx
@@ -95,10 +95,9 @@ cdef class HttpParser:
         self._last_error = None
 
     cdef _maybe_call_on_header(self):
-        if self._current_header_name is not None:
+        if self._current_header_value is not None:
             current_header_name = self._current_header_name
             current_header_value = self._current_header_value
-
             self._current_header_name = self._current_header_value = None
 
             if self._proto_on_header is not None:
@@ -107,7 +106,10 @@ cdef class HttpParser:
 
     cdef _on_header_field(self, bytes field):
         self._maybe_call_on_header()
-        self._current_header_name = field
+        if self._current_header_name is None:
+            self._current_header_name = field
+        else:
+            self._current_header_name += field

You tests should still pass :)

[youknowone]

Have you tested it?

[yohanboniface]

I do :)

[1st1]

I actually like the fix @yohanboniface proposes. Can we use it in this PR?

[youknowone]

I am sorry. I totally forgot about this PR due to other schedules. Let me try it again.

[youknowone]

I replaced the fix to @yohanboniface's work

[1st1]

Merged. Guys, a wholehearted thank you for working on this PR!

[1st1]

I've just released httptools 0.0.10 with this fix.

[youknowone]

Thanks!