tls: remove SLAB_BUFFER_SIZE#21199
tls: remove SLAB_BUFFER_SIZE #21199
Uh oh!
There was an error while loading. Please reload this page.
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
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
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
addaleax added dont-land-on-v8.x author ready 
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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
10 participants
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