-
Notifications
You must be signed in to change notification settings - Fork 30k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tls: remove SLAB_BUFFER_SIZE #21199
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/ |
Resumed build: https://ci.nodejs.org/job/node-test-pull-request/16189/ |
@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.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes