Issue with streams?

[pranaygp]

this may not be an issue with the library at all, but, I'm running into an issue with using this when I try to use a stream as my body.

I'm using the resumer package to create a readable stream from file data as so

const stream = resumer().queue(data).end()

and then trying to make a fetch:

const res = await fetch('url', {
  method: 'POST',
  headers: {
    'Content-Type': 'application/octet-stream',
    'Content-Length': data.length,
    'custom-header': 'custom value'
  },
  body: new StreamBody(stream)
})

But this causes the server I'm hitting to respond with a 400 error ('Bad request').

Doing the same thing above, but using node-fetch and just using stream directly (without wrapping it innew StreamBody(stream) for the body has my server working.

Is this a known issue/could you help me debug this? Would really appreciate it :)

[pranaygp]

More details on this:

I tried debugging by setting up a simple http2 server that just pipes the input stream to stdout:

const http2 = require('http2');

const server = http2.createServer();

server.on('stream', (stream, headers) => {
  console.log(headers)
  stream.pipe(process.stdout);
  stream.respond({
    'content-type': 'text/html',
    ':status': 200
  });
  stream.end('<h1>Hello World</h1>');
});

server.listen(8080, err => {
  console.log('started server on port 8080')
});

This first thing I found out is I needed to remove the content-Length header. If that header was in my headers object, the request never went through. Known Issue?

After removing that header:

If I make a request with a DataBody that just wraps some plain text, this works perfectly. My server logs the headers and the text.

If I make the same request but switching the body to a StreamBody and use resumer to wrap my text into a stream, the server only logs the headers. Nothing from the body is printed out to my console.

[grantila]

This is surely a bug, I'm looking at fixing it right now

[grantila]

I just added unit tests for sending a stream, and it seems to work when 'content-length' is set. I'll add tests for when not having 'content-length' set, to ensure chunked stream data can be sent.

Small detail, how about adding '' + to the data.length to ensure it becomes a string? I'll add it to the code as well, potentially this could be an issue for the http2 module.

[grantila]

I've fixed the handling of headers with upper-case characters (in http2, they are all lower-case) which probably fixed the 'Content-Type' issue. Please try latest (0.0.9) to see if the problem remains.

[grantila]

Also, numeric values will be allowed too (as your data.length). If running with TypeScript, it won't be statically allowed, but at run-time it'll be stringified.

[pranaygp]

Gotcha. Thanks for getting to this so quick. Love the good work and I'll give this a shot soon :)

…

On Sat, 28 Oct 2017, 11:09 Gustaf Räntilä, ***@***.***> wrote: Also, numeric values will be allowed too (as your data.length). If running with TypeScript, it won't be *statically* allowed, but at run-time it'll be stringified. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABtutBBei7V5jKjKLotFIzxxR3DL26Cjks5sw1HRgaJpZM4QIk4b> .

[pranaygp]

So, my server definitely now received the right headers:

{ ':authority': 'localhost:8080',
  ':path': '/',
  ':scheme': 'http',
  ':method': 'POST',
  accept: 'application/json, text/*;0.9, */*;q=0.8',
  'user-agent': 'fetch-h2/0.0.9 (+https://github.com/grantila/fetch-h2) nodejs/8.8.1 nghttp2/1.25.0 uv/1.15.0',
  'content-type': 'application/octet-stream',
  'content-length': '4862966',
  'custom-header': 'custom value' }

however, I'm still not able to log the body contents. On the server. Am I accessing the stream incorrectly on the server?

I'm basically just doing:

server.on('stream', (stream, headers) => {
  console.log(headers)
  stream.pipe(process.stdout);
  ...
});

That seems to only work when the body is not a stream

[grantila]

That looks right. I added a unit test which sends a stream to the server, which just echoes it back:
stream.pipe(stream); and that seems to work. Was only 6 bytes though, I'm wondering if that's the problem. You forward stdout somewhere, right? Or are you writing 4.8mb to the terminal? ;)

I'm starting to wonder if there's another problem. I'll add unit tests with large payloads, just to get it covered.

[grantila]

Does it work if you don't set content-length? That'll likely make Node.js (or nghttp2, I'm not sure) to use transfer-encoding: chunked. Would be good to know what the result would be. The content-length isn't needed for http per se (only if you server-side want to use that number for some purpose, e.g. create a buffer, write it a database, ...).

[pranaygp]

It wasn't an issue with the payload size. I've tried sending a much smaller dataset too, and I'm still running into the same issue.

Not setting Content-Length doesn't seem to change anything

[pranaygp]

Umm, interesting.

I looked at the test case you setup, and noticed you were using through2. I gave that a shot instead of resumer, and it worked...

[grantila]

Alright, that's... Interesting. I just filed a bug upstream because it seems to be issues with the http2 module and large (+16KiB) streams... I'll keep this open until I've gotten feedback on that bug.

[grantila]

Confirmed and with a PR. I'll keep this open until we know in what version this fix is released, after which I'll have to ensure it also works here.
Great find @pranaygp, you're part of the reason this bug will be fixed in Node.js ;)

[pranaygp]

@grantila Thanks a bunch for following up with this (and glad I could help out, haha)

There seem to be quite a few more things that still need to be implemented to get this library to full parity with node-fetch. Also, a couple things are not yet implemented (

fetch-h2/lib/fetch.ts

Lines 176 to 221 in a35903c

	stream.on( 'aborted', guard( ( ...undocumented ) =>
	{
	console.error( "Not yet handled 'aborted'", undocumented );
	} ) );
	stream.on( 'error', guard( ( err: Error ) =>
	{
	reject( err );
	} ) );
	stream.on( 'frameError', guard( ( ...undocumented ) =>
	{
	console.error("Not yet handled 'frameError'", undocumented );
	} ) );
	stream.on( 'streamClosed', guard( errorCode =>
	{
	// We'll get an 'error' event if there actually is an
	// error, but not if we got NGHTTP2_NO_ERROR.
	// In case of an error, the 'error' event will be awaited
	// instead, to get (and propagate) the error object.
	if ( errorCode === NGHTTP2_NO_ERROR )
	reject( new Error( "Stream prematurely closed" ) );
	} ) );
	stream.on( 'timeout', guard( ( ...undocumented ) =>
	{
	console.error("Not yet handled 'timeout'", undocumented );
	} ) );
	stream.on( 'trailers', guard( ( headers, flags ) =>
	{
	console.error("Not yet handled 'trailers'", headers, flags);
	} ) );
	// ClientHttp2Stream events
	stream.on( 'continue', guard( ( ...undocumented ) =>
	{
	console.error("Not yet handled 'continue'", undocumented);
	} ) );
	stream.on( 'headers', guard( ( headers, flags ) =>
	{
	console.error("Not yet handled 'headers'", headers, flags);
	} ) );

)

I'm super happy to help out with completing this implementation as I plan to use it in production. Do you have a specific thing you'd like for me to focus on?

Email me at pranay.gp@gmail.com and we can start a conversation :)

[pranaygp]

Already in master :) nodejs/node#16580

[grantila]

This is great @pranaygp but it's not released yet, so I haven't been able to test it (i.e. to ensure my unit tests work).

Regarding the missing events, I'm ok having the discussion here too. I just opened up #4 to deal with this.

[grantila]

Fix released in 0.0.11