Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 26, 2018
Merged

example: send response content-length as part of http header from server #808

merged 3 commits into from
Jan 26, 2018

Conversation

carun
Copy link

@carun carun commented Jan 13, 2018

fixes issue #681

@carun
Copy link
Author

carun commented Jan 16, 2018

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"},
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

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.

Copy link
Member

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");
Copy link
Member

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";

Copy link
Author

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 = {
Copy link
Member

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?

Copy link
Author

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 = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why static?

Copy link
Author

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.

@carun
Copy link
Author

carun commented Jan 22, 2018

ping @deanberris

Copy link
Member

@deanberris deanberris left a 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"},
Copy link
Member

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.

@carun
Copy link
Author

carun commented Jan 24, 2018

Hey @deanberris Thank you for creating a wonderful library. 😄

Copy link
Member

@deanberris deanberris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- Thanks @carun!

@deanberris deanberris merged commit f94635c into cpp-netlib:0.13-release Jan 26, 2018
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.

2 participants