Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added support for chunked http transfers #337

Merged
merged 1 commit into from

3 participants

@MarioLiebisch
Collaborator

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.

@LaurentGomila

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
Collaborator

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
Collaborator

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

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

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
Collaborator

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

@LaurentGomila
// 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
Collaborator

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

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

@MarioLiebisch
Collaborator

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

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

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

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

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

@LaurentGomila LaurentGomila merged commit 56c2eb8 into from
@MarioLiebisch MarioLiebisch deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 24, 2013
  1. @MarioLiebisch

    Added support for chunked http transfers

    MarioLiebisch authored
    sf::Http now understands transfers that are sent using Chunked Transfer
    Encoding (RFC 2616; section 3.6.1).
This page is out of date. Refresh to see the latest.
Showing with 53 additions and 5 deletions.
  1. +12 −0 include/SFML/Network/Http.hpp
  2. +41 −5 src/SFML/Network/Http.cpp
View
12 include/SFML/Network/Http.hpp
@@ -316,6 +316,18 @@ public :
////////////////////////////////////////////////////////////
void parse(const std::string& data);
+
+ ////////////////////////////////////////////////////////////
+ /// \brief Read values passed in the answer header
+ ///
+ /// This function is used by Http to extract values passed
+ /// in the response.
+ ///
+ /// \param in String stream containing the header values
+ ///
+ ////////////////////////////////////////////////////////////
+ void parseFields(std::istream &in);
+
////////////////////////////////////////////////////////////
// Types
////////////////////////////////////////////////////////////
View
46 src/SFML/Network/Http.cpp
@@ -31,6 +31,7 @@
#include <algorithm>
#include <iterator>
#include <sstream>
+#include <limits>
namespace
@@ -231,9 +232,48 @@ void Http::Response::parse(const std::string& data)
}
// Ignore the end of the first line
- in.ignore(10000, '\n');
+ in.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
// Parse the other lines, which contain fields, one by one
+ parseFields(in);
+
+ m_body.clear();
+
+ // Determine whether the transfer is chunked
+ if (toLower(getField("transfer-encoding")) != "chunked")
+ {
+ // Not chunked - just read everything at once
+ std::copy(std::istreambuf_iterator<char>(in), std::istreambuf_iterator<char>(), std::back_inserter(m_body));
+ }
+ else
+ {
+ // Chunked - have to read chunk by chunk
+ std::size_t length;
+
+ // Read all chunks, identified by a chunk-size not being 0
+ while (in >> std::hex >> length)
+ {
+ // Drop the rest of the line (chunk-extension)
+ in.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+
+ // Copy the actual content data
+ std::istreambuf_iterator<char> it(in);
+ for (std::size_t i = 0; i < length; i++)
+ m_body.push_back(*it++);
+ }
+
+ // Drop the rest of the line (chunk-extension)
+ in.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
+
+ // Read all trailers (if present)
+ parseFields(in);
+ }
+}
+
+
+////////////////////////////////////////////////////////////
+void Http::Response::parseFields(std::istream &in)
+{
std::string line;
while (std::getline(in, line) && (line.size() > 2))
{
@@ -252,10 +292,6 @@ void Http::Response::parse(const std::string& data)
m_fields[toLower(field)] = value;
}
}
-
- // Finally extract the body
- m_body.clear();
- std::copy(std::istreambuf_iterator<char>(in), std::istreambuf_iterator<char>(), std::back_inserter(m_body));
}
Something went wrong with that request. Please try again.