GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — example: send response content-length as part of http header from server by carun · Pull Request #808 · cpp-netlib/cpp-netlib

Conversation

@carun

@carun carun commented Jan 13, 2018

Copy link
Copy Markdown

fixes issue #681

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
carun added 2 commits January 13, 2018 10:07
@carun
example: send response content-length as part of http header from ser…
18cc913 
…ver to client (issue #681)
@carun
fix build warnings with member initialization order in async_message.hpp
7d70242
@carun

carun commented Jan 16, 2018

Copy link
Copy Markdown
Author

ping @deanberris
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deanberris
deanberris requested changes Jan 19, 2018
View reviewed changes
Comment thread libs/network/example/http/async_server_file_upload.cpp
void operator()(server::request const& req, const server::connection_ptr& conn) {
static std::map<std::string, std::string> headers = {
{"Connection","close"},
{"Content-Length", "0"},

@deanberris deanberris Jan 19, 2018

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@carun carun Jan 19, 2018

Copy link
Copy Markdown
Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@deanberris deanberris Jan 23, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@deanberris deanberris Jan 23, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread libs/network/example/http/async_server_file_upload.cpp Outdated
conn->write(err);
}
} else {
static std::string body("Only path allowed is /upload");

@deanberris deanberris Jan 19, 2018

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@carun carun Jan 19, 2018

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread libs/network/example/http/hello_world_async_server_with_work_queue.cpp Outdated
std::this_thread::sleep_for(std::chrono::seconds(10));

std::map<std::string, std::string> headers = {
static std::map<std::string, std::string> headers = {

@deanberris deanberris Jan 19, 2018

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@carun carun Jan 19, 2018

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread libs/network/example/http/hello_world_server.cpp Outdated
data << "Hello, " << ip << ':' << port << '!';

std::map<std::string, std::string> headers = {
static std::map<std::string, std::string> headers = {

@deanberris deanberris Jan 19, 2018

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@carun carun Jan 19, 2018

Copy link
Copy Markdown
Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@carun

carun commented Jan 22, 2018

Copy link
Copy Markdown
Author

ping @deanberris
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deanberris
deanberris requested changes Jan 23, 2018
View reviewed changes

@deanberris deanberris left a comment

Copy link
Copy Markdown
Member

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. 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread libs/network/example/http/async_server_file_upload.cpp
void operator()(server::request const& req, const server::connection_ptr& conn) {
static std::map<std::string, std::string> headers = {
{"Connection","close"},
{"Content-Length", "0"},

@deanberris deanberris Jan 23, 2018

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@carun
Implemented review comments.
7a0cbe7
@carun

carun commented Jan 24, 2018

Copy link
Copy Markdown
Author

Hey @deanberris Thank you for creating a wonderful library. 😄
glynos reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
deanberris
deanberris approved these changes Jan 26, 2018
View reviewed changes

@deanberris deanberris left a comment

Copy link
Copy Markdown
Member

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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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

Reviewers

@deanberris deanberris deanberris approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@carun @deanberris
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.