tls: remove SLAB_BUFFER_SIZE#21199
Conversation
This constant has not been in use for many years now and the test alongside it is invalid, as well as flaky.
addaleax
left a comment
There was a problem hiding this comment.
Would feel a teeny tiny bit more comfortable if the export was removed in a semver-major PR, but at the same time I find it hard to come up with a (once) valid use case that could actually be broken by this
|
@addaleax I'm fine with it being |
|
@apapirovski If the test is flaky, we probably want it gone in older branches as well, right? |
|
@addaleax I mean, it doesn't fail so often that it's a problem. I've seen it twice in six months. We don't have a thread for it. But yeah, would obviously be nice... we could prob backport just the test removal if this ends up semver-major. (It's just that there are so many more test runs on master.) |
joyeecheung
left a comment
There was a problem hiding this comment.
Should we mention it in deprecations.md as an EOL? It is not impossible that someone may expect this to be a number instead of undefined.
Can you please elaborate a bit? I don't understand why. The socket is destroyed when the |
|
@lpinca I'm pretty sure the response event can trigger before the full |
|
@apapirovski you sure? afaik the |
|
@lpinca ok, just checked and you're right on that point. I think there could still be final bits making it through though since we occasionally have empty writes in TLS for state maintenance? I would need to look in more detail but IMO something like that might be going on here. I'll do some packet inspection and see exactly what's being sent before this lands, to make sure there's not a real bug hiding. |
|
@apapirovski yes I was actually wondering if this was caused by a deeper bug. Thanks. |
|
New CI since old one 404s now: https://ci.nodejs.org/job/node-test-pull-request/16078/ |
|
@apapirovski @addaleax I believe this should be ready to land, right? Just needs another CI run (since the old ones keep becoming stale)? |
This constant has not been in use for many years now and the test alongside it is invalid, as well as flaky. PR-URL: #21199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Landed in 0aae34f |
This constant has not been in use for many years now and the test alongside it is invalid, as well as flaky. PR-URL: #21199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This constant has not been in use for many years now and the test alongside it is invalid, as well as flaky. PR-URL: nodejs/node#21199 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This constant has not been in use for many years now and the test alongside it is invalid, as well as flaky.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes