Here's my attempt at properly handling the content length header #164
Conversation
| this->part.assign('\0'); | ||
| this->response_parser_.reset(); | ||
| //NETWORK_MESSAGE("forcing socket disconnect on content length"); | ||
| //connection_delegate_->disconnect(); |
There was a problem hiding this comment.
You really don't need to force the disconnection. The connection can die a natural death.
There was a problem hiding this comment.
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.
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.
|
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. |
|
This should do it, I think. Please, let me know if there are any other changes I need to make! |
| try { | ||
| content_length_ = std::stoi(it->second); | ||
| NETWORK_MESSAGE("Content-Length: " << *content_length_); | ||
| } catch(const std::invalid_argument&) { |
There was a problem hiding this comment.
I suggest a NETWORK_MESSAGE here.
There was a problem hiding this comment.
What line number is here exactly? There's a NETWORK_MESSAGE printing the content length, for example.
There was a problem hiding this comment.
Inside the catch, print out that you got an invalid content length value.
|
Just a little more... and please make sure that all the tests still pass. :) |
|
The mime-roundtrip test fails at the moment. I'm checking to see if my changes caused this. |
|
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 Reymont notifications@github.comwrote:
Dean Michael Berris |
|
Please, go ahead and merge :-). |
Fix indefinite wait on body responses where content-length has already been provided.
|
Thanks Joel! |
No description provided.