Skip to content

http2 client stops sending after stream has reached 16KiB #16578

Description

@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

Activity

  1. added
    http2Issues and PRs related to the http2 subsystem.
    on Oct 28, 2017
  2. addaleax commented on Oct 28, 2017

    @addaleax
    Member

    @nodejs/http2

  3. apapirovski commented on Oct 28, 2017

    @apapirovski
    Contributor

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

  4. jasnell commented on Oct 28, 2017

    @jasnell
    Member

    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

  5. jasnell commented on Oct 28, 2017

    @jasnell
    Member

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

  6. apapirovski commented on Oct 28, 2017

    @apapirovski
    Contributor

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

  7. grantila commented on Oct 28, 2017

    @grantila
    Author

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

  8. apapirovski commented on Oct 28, 2017

    @apapirovski
    Contributor

    @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.

  9. grantila commented on Oct 28, 2017

    @grantila
    Author

    @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.

  10. apapirovski commented on Oct 28, 2017

    @apapirovski
    Contributor

    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.)

  11. grantila commented on Oct 28, 2017

    @grantila
    Author

    @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)

  12. apapirovski commented on Oct 28, 2017

    @apapirovski
    Contributor

    @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.

  13. jasnell commented on Oct 28, 2017

    @jasnell
    Member

    Where's the issue?

  14. apapirovski commented on Oct 29, 2017

    @apapirovski
    Contributor

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

  15. 1 remaining item

  16. added a commit that references this issue on Oct 30, 2017
  17. added a commit that references this issue on Dec 7, 2017
  18. added a commit that references this issue on Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    confirmed-bugIssues and PRs for confirmed bugs.http2Issues and PRs related to the http2 subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions