- Notifications
You must be signed in to change notification settings - Fork425
Here's my attempt at properly handling the content length header#164
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You really don't need to force the disconnection. The connection can die a natural death.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about the block below // TODO? Is it really needed? I don't understand the meaning of source and destination here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The source of the message is technically the server from which the message originated from. I used to populate this (I think) with an IP address of the remote host. The destination I leave blank because technically the destination of the response is the current connection handling the request. In the future I may put something useful there, but in the meantime the destination is kept empty.
The assigning of an empty string is unnecessary, as default-constructed strings usually use small-string optimization and will hold 16 bytes (or some similar amount) of strings. Remove that as you don't really need to do it anyway.
deanberris commentedOct 31, 2012
Thanks Joel -- if you have time to make the changes and see whether it's worth doing, that would be most appreciated. I don't think making an extra copy of the headers is a good way to solve this particular problem that's only really needed in the body parsing code. |
joelreymont commentedOct 31, 2012
This should do it, I think. Please, let me know if there are any other changes I need to make! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I suggest a NETWORK_MESSAGE here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What line number is here exactly? There's a NETWORK_MESSAGE printing the content length, for example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Inside the catch, print out that you got an invalid content length value.
deanberris commentedOct 31, 2012
Just a little more... and please make sure that all the tests still pass. :) |
joelreymont commentedOct 31, 2012
The mime-roundtrip test fails at the moment. I'm checking to see if my changes caused this. |
ghost commentedOct 31, 2012
Nope, you didn't cause this. That's fine, we know mime_roundtrip fails. I say if that's the only error, I'm happy to merge this. Let me know when you're ready. :) On Thu, Nov 1, 2012 at 1:05 AM, Joel Reymontnotifications@github.comwrote:
Dean Michael Berris |
joelreymont commentedOct 31, 2012
Please, go ahead and merge :-). |
Fix indefinite wait on body responses where content-length has already been provided.
deanberris commentedOct 31, 2012
Thanks Joel! |
No description provided.