Stream multipart/related is broken: missing chunked encoding for default GET method.

[thomas-riccardi]

Issue #1185 implemented multipart/related stream support even for streams of unknown length. This means using HTTP Transfer-Encoding: chunked because the other transfer encoding (the default one) requires setting the Content-Length header beforehand.

HTTP chunked encoding is not implemented in current request master:
Using wireshark I see that currently neither the content-length nor the transfer-encoding: chunked is set.
The combined-stream is part of the solution, but it needs to be piped to a chunk encoder before sending it to the socket. I don't know what can be done by node.js itself (with its http module).

This means that current multipart/related requests are broken HTTP requests since request 2.47.0.
(Tested with node v0.10.18 & v0.10.33 on 2.48.0 & current master)

I don't understand how and why for some people it did send chunked encoded requests (it seems to be the case here: #1243).

[simov]

This is what CouchDB receives when the Content-Length is not being set

Headers: [{'Connection',"keep-alive"},
          {'Content-Type',"multipart/related; boundary=ab95e194-58df-42f2-943e-a680489951eb"},
          {'Host',"localhost:5984"},
          {'Transfer-Encoding',"chunked"}]

This is what the test server in test-multipart receives

{ host: 'localhost:8080',
  accept: 'application/json',
  'content-type': 'multipart/related; boundary=70e7cc0f-f8dd-4d9b-98c3-0541f367f881',
  connection: 'keep-alive',
  'transfer-encoding': 'chunked' }

Passing this options

multipart: {
  chunked: false,
  data: [
     body: fs.readFileSync('cat.png')
  ]
}

Sends this to CouchDB

Headers: [{'Connection',"keep-alive"},
          {'Content-Length',"22272"},
          {'Content-Type',"multipart/related; boundary=082ea6fe-6c72-4bf0-bb29-943828397c12"},
          {'Host',"localhost:5984"}]

and this inside the tests

{ host: 'localhost:8080',
  'content-type': 'multipart/related; boundary=9af9e209-5e35-4f39-9098-34b856644333',
  'content-length': '172',
  connection: 'keep-alive' }

[thomas-riccardi]

Here is my test:

var request = require('request');

var source = 'https://registry.npmjs.org/';
var target = 'http://requestb.in/1lkya2s1';

request({
  url: target,
  qs: { test: 3 },
  multipart:[
    { body: 'test' },
    { body: request(source) }
  ]
}, function (error, response, body) {
  console.log('response', error, body);
}).on('error', function (error) {
  console.log('error:', error);
});

http://requestb.in/1lkya2s1?inspect
No chunked encoding, no content-length.

[simov]

Are you trying to make a GET request using multipart/related ? I think request defaults to a GET request.

[simov]

@triccardi-systran take a look at this http://requestb.in/rlstv7rl?inspect

The POST request at the bottom is the one I'm sending to GDrive, the PUT request at the top is the one I'm sending to CouchDB.

[thomas-riccardi]

OK, it works with method: 'POST', but not with the default GET.
It should work independently of the method, shouldn't it? method is just a semantic keyword for the request, not a descriptor of its encoding or other technical things.

[nylen]

@triccardi-systran from RFC 7231 (HTTP/1.1): Semantics and Content:

Request methods are considered "safe" if their defined semantics are
essentially read-only; i.e., the client does not request, and does
not expect, any state change on the origin server as a result of
applying a safe method to a target resource.
...
Of the request methods defined by this specification, the GET, HEAD,
OPTIONS, and TRACE methods are defined to be safe.

and

The GET method requests transfer of a current selected representation
for the target resource.

Both of these conditions are violated if you're sending a request body, so GET requests must not have bodies.

I don't think there is still an issue here, let us know if you disagree.

[thomas-riccardi]

@nylen I know some think GET requests cannot have body (and thus no multipart body, or chunked encoding), but I really don't see an contradiction between what you quoted from the RFC and the following scenario:
I want to ask the server for a specific resource (so this is covered by your second quote, and it's also safe as defined in your first quote), and use a chunked encoded multipart body to describe what resource I want.
Instance of this scenario:
A server that computes the md5, or a cryptographic signature of each part of a GET multipart/related chunked encoded request.

Sending a large request doesn't necessarily means we will modify the state of the server.
I understand it's not the usual scenario, but I don't see why I couldn't do it using standard HTTP.

Regardless of the chunk encoding, prior to 2.47.0 we could send multipart/related GET requests (albeit only for String & Buffers parts), but now request silently fails by sending invalid HTTP requests since it defaults to combined-stream, but without always using chunked-encoding: this is a regression.
At the very least we should fallback to the old implementation that works in case of String&Buffer parts, and either support stream parts, or return an error.

[simov]

@triccardi-systran you can do the same in v2.48 (use a GET request) with these options are set

multipart: {
  chunked: false,
  data: [
    {
      'content-type': 'application/json',
      body: JSON.stringify({
        _attachments: {
          'cat.png': {
            follows: true,
            content_type: 'image/png',
            length: 22025
          }
        }
      })
    },
    {
      body: fs.readFileSync(image)
    }
  ]
}

http://requestb.in/1joavk71?inspect

Besides that the current multipart implementation always set transfer-encoding:'chunked' by default (just take a look at the tests in the referenced pull request above)

So what you are suggesting is to either use combined-stream or the old implementation based on body type? What happens if I want to use chunked encoding always? Isn't the current implementation better - where you explicitly specify what you want to use (even though it defaults to chunked)?

[simov]

@triccardi-systran as for using GET request with the current implementation - if we set transfer-encoding header to chunked explicitly inside the multipart method, this will most probably solve the regression issue you are talking about.

Here is what I'm getting back from requestb.in doing so

<!DOCTYPE html>
<html>
<head>
  <meta name="viewport" content="width=device-width, initial-scale=1">
  <style type="text/css">
    html, body, iframe { margin: 0; padding: 0; height: 100%; }
    iframe { display: block; width: 100%; border: none; }
  </style>
<title>Application Error</title>
</head>
<body>
  <iframe src="//s3.amazonaws.com/heroku_pages/error.html">
    <p>Application Error</p>
  </iframe>
</body>
</html>

[simov]

The other way around is to check for

chunked && options.method == 'GET'

and throw an error saying that 'Get requests are not allowed using transfer-encoding chunked'

This will effectively eliminate the source of annoyance in this thread.

[thomas-riccardi]

@simov I know I can toggle the old implementation, but what I said is that old code worked on old request and now doesn't on new request (for String & Buffer multipart in a GET request): it's a regression. Do the request project allow regression without changing its major version number?

Then, I suggest to guarantee chunk encoding whenever we use combined-stream, this will avoid sending invalid HTTP requests.
Following your suggestion, I tested manually adding the Transfer-Encoding: chunked header on a GET request, and it worked: someone down the line did the chunked encoding (probably node http itself).
So, I propose to force add this header in request whenever the chunked option is true (even when it's just the default value). This will make multipart stream always work, and also remove the current regression. The tests should also be updated to test with GET & POST.

Otherwise, If you don't want to support GET requests with body, then it should be documented and request should return an error in this case, not silently failing by sending an invalid HTTP request.

[simov]

Yep that's the suggestion from my second comment. We should either throw an error or send a correct chunked request at all time.

Let's see if anyone else have something to add, also I want my #1275 PR reviewed first, because I'm going to work on top of that.

Once this one is in, I'll add the tests for GET request and will make sure we have the Transfer-Encoding: chunked header set by default.

[thomas-riccardi]

@simov perfect, thanks for your help!

I expect no objection to this, because if someone doesn't want to send GET request with a body, then just don't do it.

[nylen]

@triccardi-systran fair enough, I agree that your example is unusual but valid according to the RFC. so the proposed solution to always set transfer-encoding sounds good to me.

[simov]

Great, I'll push later today.