Repository navigation
HTTPS highwatermark not working #33262
Description
- 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.
Activity
rickyes commentedon May 6, 2020
Ok, I will create a new pr associated with this issue.
rickyes commentedon May 6, 2020
I found out that
http.requestalso receives chunks of data of a fixed 64KB size. I'm looking for what went wrong.rickyes commentedon May 7, 2020
Unfortunately, the
readableHighWaterMarkparameter may not be supported. The readableHighWaterMark parameter is not supported in http.request and https.requestthunderol commentedon May 7, 2020
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 commentedon May 8, 2020
That 16k is the TLS record size, its size is fixed by the protocol.
I don't think
readableHighWaterMarkcan be made meaningful for TLS/HTTPS because that 16k will still be sitting in a buffer somewhere.BridgeAR commentedon May 8, 2020
AFAIK it is possible to reduce the maximum record size below 16kb but not to increase it. We should likely document that.
bnoordhuis commentedon May 8, 2020
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_lengthTLS extension that allows negotiating a smaller maximum record size but it has Issues and node never uses it. Practically speaking, the record size is 16k1 - 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.
1 16k + 5 in fact
puzpuzpuz commentedon May 13, 2020
Considering what's written above, shouldn't we revert 58682d8 or, at least, document the 16KB upper limit?
bnoordhuis commentedon May 13, 2020
@puzpuzpuz Reverting and documenting why it's not supported sounds good to me.
puzpuzpuz commentedon May 13, 2020
@bnoordhuis @BridgeAR
I've created the revert PR: #33387. It also includes a short note in
tlsdocs.benjamingr commentedon May 13, 2020
Is there a use case for setting a highWatermark of less than 16K and if there is should that be supported?
benjamingr commentedon May 13, 2020
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 commentedon May 23, 2020
This should have been fixed by b51d1cf. I guess this should be closed?
benjamingr commentedon May 23, 2020
@BridgeAR probably only after #33346 has concluded
BridgeAR commentedon May 23, 2020
@benjamingr that seems independent from this issue to me?
benjamingr commentedon May 23, 2020
@BridgeAR the OP of that PR only opened that PR after #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).
bnoordhuis commentedon Dec 29, 2022
I don't think there's anything left to do, all the linked issues and pull requests have been closed.