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

Added support for chunked http transfers #337

Merged
merged 1 commit into from Oct 24, 2013

Conversation

Projects
None yet
3 participants
@MarioLiebisch
Member

MarioLiebisch commented Jan 13, 2013

sf::Http now understands transfers sent using Chunked Transfer Encoding (RFC 2616; section 3.6.1). This is a patch addressing issue #296 (tested with PHP so far).

Moved this (request #324) to its own branch to keep it clean of further commits. Sorry for any inconvenience.

@ghost ghost assigned LaurentGomila Sep 14, 2013

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 2, 2013

Member

Thanks.

Let's do some cleaning before I integrate it:

  1. Your implementation of copy_n is not the clearest to read, I would write it like this:
template<class InputIterator, class Size, class OutputIterator> OutputIterator copy_n(InputIterator in, Size n, OutputIterator out)
{
    for (Size i = 0; i < n; ++i)
        *out++ = *in++;
    return out;
}

But since it is called once, we don't need to factorize it.

  1. I would rather factorize the loop that parses header fields, since you now need a second one after you parse the chunks.
  2. std::string dummyis never used and can be removed.
  3. Why do you use std::string::compare rather than operator==?
  4. Has it been tested thoroughly, with some non-trivial scenarios?
Member

LaurentGomila commented Oct 2, 2013

Thanks.

Let's do some cleaning before I integrate it:

  1. Your implementation of copy_n is not the clearest to read, I would write it like this:
template<class InputIterator, class Size, class OutputIterator> OutputIterator copy_n(InputIterator in, Size n, OutputIterator out)
{
    for (Size i = 0; i < n; ++i)
        *out++ = *in++;
    return out;
}

But since it is called once, we don't need to factorize it.

  1. I would rather factorize the loop that parses header fields, since you now need a second one after you parse the chunks.
  2. std::string dummyis never used and can be removed.
  3. Why do you use std::string::compare rather than operator==?
  4. Has it been tested thoroughly, with some non-trivial scenarios?
@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 3, 2013

Member

Will look over it probably later today or tomorrow. copy_n was more like a quick fix due to it not being available everywhere (don't remember; I think it's been MSVC only). There might be a better solution now. Fourth question: Good question! Fifth question: Only with PHP outputting different files through it's output buffering; will see whether there are some public test cases or some servers to find using this.

Member

MarioLiebisch commented Oct 3, 2013

Will look over it probably later today or tomorrow. copy_n was more like a quick fix due to it not being available everywhere (don't remember; I think it's been MSVC only). There might be a better solution now. Fourth question: Good question! Fifth question: Only with PHP outputting different files through it's output buffering; will see whether there are some public test cases or some servers to find using this.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 22, 2013

Member

Finally updated the branch, let me know in case you'd like anything else changed - will squash everything once done (also just noticed an added line break at the end).

Edit: Code is tested and works with real web pages as well as the example provided by http://jigsaw.w3.org/HTTP/ - just requires the HTML header to be set to version 1.1 (not sure the default value should be changed, as there might be other stuff not supported yet).

Member

MarioLiebisch commented Oct 22, 2013

Finally updated the branch, let me know in case you'd like anything else changed - will squash everything once done (also just noticed an added line break at the end).

Edit: Code is tested and works with real web pages as well as the example provided by http://jigsaw.w3.org/HTTP/ - just requires the HTML header to be set to version 1.1 (not sure the default value should be changed, as there might be other stuff not supported yet).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 22, 2013

Member

Thanks, I'm glad you found time to work on it :)

Just a few minor details:

  • as I said in my first post, here copy_n is probably overdesigned, just put the (simpler) code at the single location where it is called.
  • I would rename readVars parseFields, but that's just my own personal taste, and it would take a more generic std::istream argument ;)

After that I'll happily merge your contribution.

Member

LaurentGomila commented Oct 22, 2013

Thanks, I'm glad you found time to work on it :)

Just a few minor details:

  • as I said in my first post, here copy_n is probably overdesigned, just put the (simpler) code at the single location where it is called.
  • I would rename readVars parseFields, but that's just my own personal taste, and it would take a more generic std::istream argument ;)

After that I'll happily merge your contribution.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 22, 2013

Member

Hum and your code is not based on the current HEAD (look at the HTTPS code: now it's explicitely disabled instead of pretending it is supported). I think it would be better if you rebased your branch on the current HEAD, to avoid potential problems.

Member

LaurentGomila commented Oct 22, 2013

Hum and your code is not based on the current HEAD (look at the HTTPS code: now it's explicitely disabled instead of pretending it is supported). I think it would be better if you rebased your branch on the current HEAD, to avoid potential problems.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 23, 2013

Member

There you go (I'm actually surprised how easy the rebase worked this time :)).

Member

MarioLiebisch commented Oct 23, 2013

There you go (I'm actually surprised how easy the rebase worked this time :)).

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 24, 2013

Member
// Copy the actual content data
std::istreambuf_iterator<char> iin(in);
std::back_insert_iterator<std::string> iout(m_body);

for (size_t i = 0; i < length; i++)
    *iout++ = *iin++;

I was sure you would write it this way :D

std::back_insert_iterator is an adaptor that is meant to be used in generic algorithms. Here you're not in a generic context, so why not using directly the underlying function (m_body.push_back)?

For std::istreambuf_iterator it's a little different since what it does internally is more complex.

You also have a type mismatch: length is an unsigned long while i is a std::size_t (by the way, don't forget to use std:: for standard types).

// Copy the actual content data
std::istreambuf_iterator<char> it(in);
for (unsigned long i = 0; i < length; i++)
    m_body.push_back(*it++);

Again I bother you with a minor modification, but this is how I work, every little part of what I add to the code is important and must remain consistent with my personal conventions.

Member

LaurentGomila commented Oct 24, 2013

// Copy the actual content data
std::istreambuf_iterator<char> iin(in);
std::back_insert_iterator<std::string> iout(m_body);

for (size_t i = 0; i < length; i++)
    *iout++ = *iin++;

I was sure you would write it this way :D

std::back_insert_iterator is an adaptor that is meant to be used in generic algorithms. Here you're not in a generic context, so why not using directly the underlying function (m_body.push_back)?

For std::istreambuf_iterator it's a little different since what it does internally is more complex.

You also have a type mismatch: length is an unsigned long while i is a std::size_t (by the way, don't forget to use std:: for standard types).

// Copy the actual content data
std::istreambuf_iterator<char> it(in);
for (unsigned long i = 0; i < length; i++)
    m_body.push_back(*it++);

Again I bother you with a minor modification, but this is how I work, every little part of what I add to the code is important and must remain consistent with my personal conventions.

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 24, 2013

Member

Done. I'm sometimes lazy, how did you know? :P But besides that, no worries, I'm doing these patches to learn and try out new things that don't fit into work projects, so just keep going. :)

Member

MarioLiebisch commented Oct 24, 2013

Done. I'm sometimes lazy, how did you know? :P But besides that, no worries, I'm doing these patches to learn and try out new things that don't fit into work projects, so just keep going. :)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 24, 2013

Member

Why std::string::size_type everywhere? Just keep std::size_t, don't make things look complicated where they are not 😑

Member

LaurentGomila commented Oct 24, 2013

Why std::string::size_type everywhere? Just keep std::size_t, don't make things look complicated where they are not 😑

@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 24, 2013

Member

I'd consider it more clean, as we're working with strings here. Technically it's usually all the same, so change it? Hmm...

Member

MarioLiebisch commented Oct 24, 2013

I'd consider it more clean, as we're working with strings here. Technically it's usually all the same, so change it? Hmm...

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 24, 2013

Member

Yes of course these types are all the same and it won't change anything, but here we are talking about clean and readable code.

Using this type would be clean if the number was coming from the string itself (like the result of std::string::find), in this case it would make perfect sense. But here we are just reading a number from a stream, we could do anything else with it rather than (or additionnally to) adding this amount of bytes to a string. So the type should reflect that we are reading an unsigned number, nothing else.

If you want to go this way and use std::string::size_type because it's cleaner, then you have to handle it correctly from the start: use the largest available type for reading, then check it against the max value of std::string::size_type (according to the current number of characters already stored in the string), then cast it, etc. But of course we know that the number will be small and we don't care about all this stuff at all, so we just do it simple so that the next time someone reads this code he doesn't wonder why in this context we are using such a specific type ;)

Member

LaurentGomila commented Oct 24, 2013

Yes of course these types are all the same and it won't change anything, but here we are talking about clean and readable code.

Using this type would be clean if the number was coming from the string itself (like the result of std::string::find), in this case it would make perfect sense. But here we are just reading a number from a stream, we could do anything else with it rather than (or additionnally to) adding this amount of bytes to a string. So the type should reflect that we are reading an unsigned number, nothing else.

If you want to go this way and use std::string::size_type because it's cleaner, then you have to handle it correctly from the start: use the largest available type for reading, then check it against the max value of std::string::size_type (according to the current number of characters already stored in the string), then cast it, etc. But of course we know that the number will be small and we don't care about all this stuff at all, so we just do it simple so that the next time someone reads this code he doesn't wonder why in this context we are using such a specific type ;)

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 24, 2013

Member

... so basically you got it wrong in parseFields: that one should really be std::string::size_type...

Member

LaurentGomila commented Oct 24, 2013

... so basically you got it wrong in parseFields: that one should really be std::string::size_type...

Added support for chunked http transfers
sf::Http now understands transfers that are sent using Chunked Transfer
Encoding (RFC 2616; section 3.6.1).
@MarioLiebisch

This comment has been minimized.

Show comment
Hide comment
@MarioLiebisch

MarioLiebisch Oct 24, 2013

Member

Okay, I never really liked "check all the things" anyway... ;)

Member

MarioLiebisch commented Oct 24, 2013

Okay, I never really liked "check all the things" anyway... ;)

LaurentGomila added a commit that referenced this pull request Oct 24, 2013

Merge pull request #337 from MarioLiebisch/chunked-http
Added support for chunked HTTP transfers

@LaurentGomila LaurentGomila merged commit 56c2eb8 into SFML:master Oct 24, 2013

@MarioLiebisch MarioLiebisch deleted the MarioLiebisch:chunked-http branch Oct 24, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment