- Notifications
You must be signed in to change notification settings - Fork425
example: send response content-length as part of http header from server#808
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
ping@deanberris |
| @@ -191,6 +191,7 @@ struct connection_handler { | |||
| void operator()(server::request const& req, const server::connection_ptr& conn) { | |||
| static std::map<std::string, std::string> headers = { | |||
| {"Connection","close"}, | |||
| {"Content-Length", "0"}, | |||
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.
Why do you need this?
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.
This was the reason for#681
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 think I understand now, that you want to always define this for every new response. That makes sense.
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 think this map really ought to not be static, unless you're going to make copies of it for every instance of this handler -- note that there could be multiple threads accessing this map, and updating it on the fly will cause all sorts of threading issues. Aside from that, the other changes look good.
| conn->write(err); | ||
| } | ||
| } else { | ||
| static std::string body("Only path allowed is /upload"); |
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.
It's usually a bad idea to have function-local static objects with non-trivial destructors. Consider just turning this into a character array that's also constexpr:
static constexpr char body[] = "Only path allowed is /upload";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.
sure, will fix it.
| @@ -125,13 +125,16 @@ void process_request(work_queue& queue) { | |||
| // some heavy work! | |||
| std::this_thread::sleep_for(std::chrono::seconds(10)); | |||
| std::map<std::string, std::string> headers = { | |||
| static std::map<std::string, std::string> headers = { | |||
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.
Why does this need to be static?
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.
so that the map doesn't get created every time the function is called.
| @@ -30,13 +30,17 @@ struct hello_world { | |||
| std::ostringstream data; | |||
| data << "Hello, " << ip << ':' << port << '!'; | |||
| std::map<std::string, std::string> headers = { | |||
| static std::map<std::string, std::string> headers = { | |||
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.
Same here, why static?
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.
ditto.. if it is not required, I can remove it.
ping@deanberris |
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.
Just one more change, and I think we're good to go. Thanks for the patience, vacation and travel taking too much time for me recently. 😄
| @@ -191,6 +191,7 @@ struct connection_handler { | |||
| void operator()(server::request const& req, const server::connection_ptr& conn) { | |||
| static std::map<std::string, std::string> headers = { | |||
| {"Connection","close"}, | |||
| {"Content-Length", "0"}, | |||
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 think this map really ought to not be static, unless you're going to make copies of it for every instance of this handler -- note that there could be multiple threads accessing this map, and updating it on the fly will cause all sorts of threading issues. Aside from that, the other changes look good.
Hey@deanberris Thank you for creating a wonderful library. 😄 |
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.
LGTM -- Thanks@carun!
fixes issue#681