Skip to content

HTTPS highwatermark not working #33262

Closed as not planned
Closed as not planned
HTTPS highwatermark not working#33262
Labels
docIssues and PRs related to Node.js documentation.httpsIssues and PRs related to the https subsystem.

Description

@thunderol
thunderol
opened on May 6, 2020
  • 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

Activity

  1. rickyes commented on May 6, 2020

    @rickyes
    rickyes
    on May 6, 2020
    Contributor

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

  2. rickyes commented on May 6, 2020

    @rickyes
    rickyes
    on May 6, 2020
    Contributor

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

  3. rickyes commented on May 7, 2020

    @rickyes
    rickyes
    on May 7, 2020
    Contributor

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

  4. thunderol commented on May 7, 2020

    @thunderol
    thunderol
    on May 7, 2020
    Author

    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?

  5. bnoordhuis commented on May 8, 2020

    @bnoordhuis
    bnoordhuis
    on May 8, 2020
    Member

    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.

  6. BridgeAR commented on May 8, 2020

    @BridgeAR
    BridgeAR
    on May 8, 2020
    Member

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

  7. BridgeAR
    added
    docIssues and PRs related to Node.js documentation.
    on May 8, 2020

  8. bnoordhuis commented on May 8, 2020

    @bnoordhuis
    bnoordhuis
    on May 8, 2020
    Member

    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 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
  9. ronag
    added
    httpsIssues and PRs related to the https subsystem.
    on May 10, 2020

  10. puzpuzpuz commented on May 13, 2020

    @puzpuzpuz
    puzpuzpuz
    on May 13, 2020
    Member

    Considering what's written above, shouldn't we revert 58682d8 or, at least, document the 16KB upper limit?

  11. bnoordhuis commented on May 13, 2020

    @bnoordhuis
    bnoordhuis
    on May 13, 2020
    Member

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

  12. puzpuzpuz commented on May 13, 2020

    @puzpuzpuz
    puzpuzpuz
    on May 13, 2020
    Member

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

  13. benjamingr commented on May 13, 2020

    @benjamingr
    benjamingr
    on May 13, 2020
    Member

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

  14. benjamingr commented on May 13, 2020

    @benjamingr
    benjamingr
    on May 13, 2020
    Member

    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?

  15. BridgeAR commented on May 23, 2020

    @BridgeAR
    BridgeAR
    on May 23, 2020
    Member

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

  16. benjamingr commented on May 23, 2020

    @benjamingr
    benjamingr
    on May 23, 2020
    Member

    @BridgeAR probably only after #33346 has concluded

  17. BridgeAR commented on May 23, 2020

    @BridgeAR
    BridgeAR
    on May 23, 2020
    Member

    @benjamingr that seems independent from this issue to me?

  18. benjamingr commented on May 23, 2020

    @benjamingr
    benjamingr
    on May 23, 2020
    Member

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

  19. bnoordhuis commented on Dec 29, 2022

    @bnoordhuis
    bnoordhuis
    on Dec 29, 2022
    Member

    I don't think there's anything left to do, all the linked issues and pull requests have been closed.

  20. bnoordhuis
    closed this as not plannedon Dec 29, 2022
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

    docIssues and PRs related to Node.js documentation.httpsIssues and PRs related to the https subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions