Uh oh!
There was an error while loading. Please reload this page.
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.
apapirovski
commented
Jun 7, 2018
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
apapirovski
commented
Jun 7, 2018
@addaleax I'm fine with it being |
addaleax
commented
Jun 7, 2018
@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.
lpinca
commented
Jun 9, 2018
Can you please elaborate a bit? I don't understand why. The socket is destroyed when the |
apapirovski
commented
Jun 9, 2018
@lpinca I'm pretty sure the response event can trigger before the full |
lpinca
commented
Jun 9, 2018
@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. |
lpinca
commented
Jun 10, 2018
@apapirovski yes I was actually wondering if this was caused by a deeper bug. Thanks. |
addaleax
commented
Jul 18, 2018
maclover7
commented
Jul 30, 2018
New CI since old one 404s now: https://ci.nodejs.org/job/node-test-pull-request/16078/ |
targos
commented
Aug 4, 2018
Resumed build: https://ci.nodejs.org/job/node-test-pull-request/16189/ |
maclover7
commented
Aug 11, 2018
@apapirovski@addaleax I believe this should be ready to land, right? Just needs another CI run (since the old ones keep becoming stale)? |
jasnell
commented
Aug 12, 2018
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>
jasnell
commented
Aug 12, 2018
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