Node: HTTPS highwatermark not working

[thunderol]

• Version: Node.js v15.0.0-nightly20200505c17dcb3253
• Platform: Windows 10 x64
• Subsystem: https

What steps will reproduce the bug?

In #32781 was fixed that highWatermark was not established in https streams when we passed that option. But after some tests it is still receiving chunks of data of a fixed 16KB size.

Downloading a file from a LAN Server via HTTP with an increased hightwatermark we can see this progress (chunks of ~65kbs)

HTTP Test
--------------------------------------------------
Response.readableHighWaterMark: 131072
Outfile WriteStream WritableHighWaterMark: 131072

Progress: 0%    10.69 kB / 21.85 MB
Progress: 0%    74.69 kB / 21.85 MB
Progress: 0%    138.69 kB / 21.85 MB
Progress: 0%    202.69 kB / 21.85 MB
Progress: 1%    266.69 kB / 21.85 MB
Progress: 1%    330.69 kB / 21.85 MB
Progress: 1%    394.69 kB / 21.85 MB
Progress: 2%    458.69 kB / 21.85 MB
Progress: 2%    522.69 kB / 21.85 MB
Progress: 2%    586.69 kB / 21.85 MB
...

Downloading the same file, from the same LAN Server via HTTPS we are still receiving chunks of 16KB even traces of the response readadableHighWaterMark are established to 128KB, so it seems there is something that is not propagating well.

HTTPS Test
--------------------------------------------------
Response.readableHighWaterMark: 131072
Outfile WriteStream WritableHighWaterMark: 131072

Progress: 0%    16.00 kB / 21.85 MB
Progress: 0%    32.00 kB / 21.85 MB
Progress: 0%    48.00 kB / 21.85 MB
Progress: 0%    64.00 kB / 21.85 MB
Progress: 0%    80.00 kB / 21.85 MB
Progress: 0%    96.00 kB / 21.85 MB
Progress: 0%    112.00 kB / 21.85 MB
...

How often does it reproduce? Is there a required condition?

Always

Additional information

Attached goes a js what Im using to do those tests.

downloadfile.test.js.txt

[rickyes]

Ok, I will create a new pr associated with this issue.

[rickyes]

I found out that http.request also receives chunks of data of a fixed 64KB size. I'm looking for what went wrong.

[rickyes]

Unfortunately, the readableHighWaterMark parameter may not be supported. The readableHighWaterMark parameter is not supported in http.request and https.request

[thunderol]

aucchhh. I will look the proposal of that issue. Anyway, there is an inconsistency between maximun 64Kb buffer in HTTP vs 16KB in HTTPS, isnt it?

[bnoordhuis]

That 16k is the TLS record size, its size is fixed by the protocol.

I don't think readableHighWaterMark can be made meaningful for TLS/HTTPS because that 16k will still be sitting in a buffer somewhere.

[BridgeAR]

AFAIK it is possible to reduce the maximum record size below 16kb but not to increase it. We should likely document that.

[bnoordhuis]

Yes and no. That 16k is the maximum record size. It can be up to 18k after decompression actually but node doesn't use compression.

There's a max_fragment_length TLS extension that allows negotiating a smaller maximum record size but it has Issues and node never uses it. Practically speaking, the record size is 16k¹ - that's what peers can send and that's what node accepts.

Peers can of course send smaller records but there's no way to enforce that. It'd be a protocol violation.

¹ 16k + 5 in fact

[puzpuzpuz]

Considering what's written above, shouldn't we revert https://github.com/nodejs/node/commit/58682d823acb0e566f16d1d8b33b83dfebf3aa5d or, at least, document the 16KB upper limit?

[bnoordhuis]

@puzpuzpuz Reverting and documenting why it's not supported sounds good to me.

[puzpuzpuz]

@bnoordhuis @BridgeAR
I've created the revert PR: #33387. It also includes a short note in tls docs.

[benjamingr]

Is there a use case for setting a highWatermark of _less_ than 16K and if there is should that be supported?

[benjamingr]

Also, can someone explain why TLS having a record size of 16K means the highWatermark can at most be 16K?

I would naively expect if anything for it to have to be _at least_ 16K since that's the amount required to understand a single record. I have this backwards right?

[BridgeAR]

This should have been fixed by b51d1cf. I guess this should be closed?

[benjamingr]

@BridgeAR probably only after https://github.com/nodejs/node/pull/33346 has concluded

[BridgeAR]

@benjamingr that seems independent from this issue to me?

[benjamingr]

@BridgeAR the OP of that PR only opened that PR after https://github.com/nodejs/node/issues/30107 . If I understand correctly this is a real issue (HTTPS watermark really _isn't_ working and this isn't supported as the OP shows).