http2 client stops sending after stream has reached 16KiB

[grantila]

• Version: 8.8.0
• Platform: macOS High Sierra
• Subsystem: http2

When piping a readable stream to an http2 request, it sends up to 16KiB of data, then sends no more. This can be reproduced by:

The following test server will write a file when anyone connects and sends data, using readable.pipe(writable). Usage: node server.js /tmp/foo to write /tmp/foo

const http2 = require('http2');
const fs = require('fs');

const server = http2.createServer();

const filename = process.argv[2];

server.on('stream', (stream, headers) => {
	console.log(headers);
	stream.pipe(fs.createWriteStream(filename));

	setTimeout(() => {
		stream.respond({
			'content-type': 'text/plain',
			':status': 200
		});
		stream.end("1 second has elapsed");
	}, 1000);
});

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

The following test program reads a file and connects to an http2 server and streams the file content, also using readable.pipe(writable). Usage node client.js file-to-read.

This will succeed (small file): node client.js /etc/hosts
This will fail (large file): node client.js /usr/bin/ssh

const http2 = require('http2');
const {createReadStream} = require('fs');

const session = http2.connect("http://localhost:54321");
const req = session.request({
	':path': '/',
	':method': 'POST',
}, {
	endStream: false,
});

const filename = process.argv[process.argv.length-1];
createReadStream(filename).pipe(req);
req.pipe(process.stdout);

• Expected behaviour: Stream handling should just work with the rest of Node.

#16213 might be related

[addaleax]

@nodejs/http2

[apapirovski]

Looking into it, I was pretty sure we had a test for this (since I wrote it) but will check.

[jasnell]

This likely has to do with the flow control mechanism. The client will pause sending if a window update is not received. I will investigate further, but it would be good to know if this works on 8.7, 8.6 and 8.5. there have been some changes in the flow control recently

[jasnell]

Specifically, I'm wondering if there's a race condition that's causing a window update to not be sent...

[apapirovski]

The window should be 64kb though, this seems like it sends one frame... not even the full window.

[grantila]

@jasnell I'll test these versions right now
@apapirovski Stream default highWaterMark is 16KiB, isn't it?

[apapirovski]

@grantila I think @jasnell is talking specifically about http2 flow control. We do have a test for something similar to this. I'll dig into what exactly is going on.

[grantila]

@jasnell @apapirovski When the server runs 8.5.0, 8.6.0 and 8.7.0, the result file ends up at 64KiB. In 8.8.0 it becomes 16KiB. The client version seems irrelevant, I might be wrong.

[apapirovski]

This isn't flow control, it's something about the core API and it works fine with the compatibility API. Looking into it.

(The test mentioned above runs against the compatibility API hence not noticing this regression.)

[grantila]

@apapirovski right, tests for the core API would indeed be sweet. But don't misunderstand my findings, it's not necessarily a regression. The file I tried to send was +2mb, so it failed in all versions, just with different limits (64 vs 16 k)

[apapirovski]

@grantila yep, it's not a regression (other than the fact that it's now more obvious because of the flow control changes on the C++ end). I've got the cause. Should have a PR shortly.

[jasnell]

Where's the issue?

[apapirovski]

@jasnell Sorry, took off to get dinner but here's the PR: #16580