NotificationsYou must be signed in to change notification settings
Fork425
Star2k

Introduced new chunked transfer encoding parser to remove chunk markers#720

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

Jump to bottom

Merged

deanberris merged 7 commits intocpp-netlib:masterfromumennel:master-integration

Mar 31, 2017

Merged

Introduced new chunked transfer encoding parser to remove chunk markers#720

deanberris merged 7 commits intocpp-netlib:masterfromumennel:master-integration

Mar 31, 2017

Conversation

Copy link

umennel commentedDec 14, 2016

I introduced a new client option called remove_chunk_markers. With this option selected, a new chunked transfer encoding parser is enabled which is able to parse a stream of small chunks and incrementally remove the chunk markers. The parser requires to keep internal state, so I had to parser function with a function object.
I selected the new option in some of the tests to cover the new code. Proper unit tests are missing yet.
Compiled with gcc 5.4 only.
Some issues might be worth to discuss a bit further:

How to handle parse errors (see BOOST_ASSERTs in the parser code)?
Performance??

umennel added2 commits

December 14, 2016 10:25

Introduced new chunked transfer encoding parser to remove chunk marke…

673b853

…rs in streaming clients.

Applied new remove chunk markers option to tests.

c4d758a

deanberris reviewed

Dec 15, 2016

View reviewed changes

Copy link

Member

deanberris left a comment

Choose a reason for hiding this comment

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

Thanks for this@umennel! This has been one of the most-asked-for features to be added to the client and neither@glynos nor I had the time to make it happen.

boost/network/protocol/http/client/connection/async_normal.hpp Outdated


		chunk_encoding_parser() : state(state_t::header), chunk_size(0) {}

		enumstate_t { header, header_end, data, data_end };

Copy link

Member

deanberrisDec 15, 2016

Choose a reason for hiding this comment

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

I think you can use anenum class here instead.

boost/network/protocol/http/client/connection/async_normal.hpp Outdated

		size_t chunk_size;
		std::array<typename char_<Tag>::type,1024> buffer;

		voidupdate_chunk_size(boost::iterator_range<typename std::array<typename char_<Tag>::type,1024>::const_iterator>const& range) {

Copy link

Member

deanberrisDec 15, 2016

Choose a reason for hiding this comment

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

Yeah, this is a really long line. We prefer usingclang-format to make sure we have consistent formatting in the project, so if you don't mind running this patch through it, that would be really good for readability.

boost/network/protocol/http/client/connection/async_normal.hpp Outdated

		ss << std::hex << range;
		size_t size;
		ss >> size;
		chunk_size = (chunk_size << (range.size()*4)) + size;

Copy link

Member

deanberrisDec 15, 2016

Choose a reason for hiding this comment

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

Can you help me understand this math?

Ifrange has a size of 1024, this meanschunk_size is shifted left 4096 times?

Copy link

Author

umennelDec 18, 2016

Choose a reason for hiding this comment

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

Yes, but range must correspond to the number of digits and 1024 digits would be an unrealistically large number (it would not throw an error though cause there is no overflow checking). I replaced the '+' with a '|' to make it more clear that this expression is just about appending the new bits to the lower end (and shifting the existing bits by the number of the new bits). Every hex digit corresponds to a quadruple of bits, that's why i multiply the number of digits by four. I didn't want to introduce another char buffer to accumulate the hex digits, so I decided to incrementally compute the chunk size.

boost/network/protocol/http/client/connection/async_normal.hpp Outdated

		// }

		// return body;
		// }

Copy link

Member

deanberrisDec 15, 2016

Choose a reason for hiding this comment

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

Consider just removing these lines?

libs/network/test/http/client_get_different_port_test.cpp

		typename client::options options;
		options.remove_chunk_markers(true);
		client client_;
		typename TypeParam::requestrequest("http://www.boost.org:80/");

Copy link

Member

deanberrisDec 15, 2016

Choose a reason for hiding this comment

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

I know this might be out-of-scope, but could you make this point tocpp-netlib.org instead? That would be great, thanks. :)

Copy link

Author

umennelDec 16, 2016

Choose a reason for hiding this comment

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

Unfortunately cpp-netlib.org does not yield chunk encoded content. It does not matter much for this particular test, but for the streaming test we'd have all scenarios covered withwww.boost.org: it yields chunk encoded content for http/1.1 clients and non encoded content for http/1.0 clients.

Copy link

Member

deanberrisDec 16, 2016

Choose a reason for hiding this comment

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

Ah, good point, thanks.

umennel added5 commits

December 15, 2016 23:43

Nicer formatting

e9bf5f7

Use strongly typed enum.

9586a81

Clarified and commented expression.

c70f774

Removed commented lines.

ff5c7c8

Nicer formatting and clean ups

192d520

Copy link

Author

umennel commentedMar 30, 2017

Is there any chance to merge this PR in the near future? Is there still something missing?

Copy link

Member

deanberris commentedMar 31, 2017

Hi@umennel -- I'm so sorry about the delay, this just slipped through the cracks. :(

Let me have a quick look and merge.

Copy link

Member

deanberris commentedMar 31, 2017

LGTM

deanberris merged commit03f6d3b intocpp-netlib:master

Mar 31, 2017

umennel mentioned this pull request

Mar 5, 2018

Streaming API does not allow access to the HTTP status or the headers from the callback#827

Open

igorpeshansky reviewed

Mar 7, 2018

View reviewed changes

boost/network/protocol/http/client/connection/async_normal.hpp

		body_string.append(this->part.begin(),this->part.begin() + bytes_transferred);
		if (this->is_chunk_encoding) {
		this->body_promise.set_value(parse_chunk_encoding(body_string));
		if (this->is_chunk_encoding && remove_chunk_markers_) {

Copy link

igorpeshanskyMar 7, 2018•
edited
Loading

Choose a reason for hiding this comment

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

@umennel Careful -- this actually breaks previous behavior. Now, unless you setremove_chunk_markers totrue, the following code will produce a chunked-encoded string:

http::client client;http::client::requestrequest("http://www.boost.org:80/");http::client::response response = client.get(request);std::cerr << body(response) << std::endl;

@deanberris Is this what we want?

To put it another way, can you think of a case where we wouldn't wantremove_chunk_markers to default totrue?

Copy link

Author

umennelMar 15, 2018

Choose a reason for hiding this comment

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

@igorpeshansky, indeed, the default should be true. Otherwise it is hard to keep the behavior consistent. We should decide on this before merging into 0.13.

Copy link

igorpeshanskyMar 15, 2018

Choose a reason for hiding this comment

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

Great, I'll send a PR shortly.

Copy link

igorpeshanskyMar 15, 2018

Choose a reason for hiding this comment

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

Sent#830.

igorpeshansky mentioned this pull request

Mar 15, 2018

Enable remove_chunk_markers by default. Fix a bug and add a comment.#830

Merged

Labels

None yet

Movatterモバイル変換

Introduced new chunked transfer encoding parser to remove chunk markers#720

Introduced new chunked transfer encoding parser to remove chunk markers#720

Uh oh!

Conversation

umennel commentedDec 14, 2016

Uh oh!

deanberris left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

umennel commentedMar 30, 2017

Uh oh!

deanberris commentedMar 31, 2017

Uh oh!

deanberris commentedMar 31, 2017

Uh oh!

igorpeshanskyMar 7, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

igorpeshanskyMar 7, 2018•
edited
Loading