Skip to content
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

Conversation

apapirovski
Copy link
Member

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This constant has not been in use for many years now and the test
alongside it is invalid, as well as flaky.
@apapirovski apapirovski added the tls Issues and PRs related to the tls subsystem. label Jun 7, 2018
@apapirovski
Copy link
Member Author

Copy link
Member

@addaleax addaleax left a 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

@apapirovski
Copy link
Member Author

@addaleax I'm fine with it being semver-major if that's what everyone decides. This test very infrequently fails with ECONNRESET (since it destroys the socket at a weird time) so I just want it gone from master... 😄

@addaleax
Copy link
Member

addaleax commented Jun 7, 2018

@apapirovski If the test is flaky, we probably want it gone in older branches as well, right?

@apapirovski
Copy link
Member Author

apapirovski commented Jun 7, 2018

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

Copy link
Member

@joyeecheung joyeecheung left a 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.

@lpinca
Copy link
Member

lpinca commented Jun 9, 2018

@apapirovski

This test very infrequently fails with ECONNRESET (since it destroys the socket at a weird time)

Can you please elaborate a bit? I don't understand why. The socket is destroyed when the 'response' event is emitted. At this point no more data is written to the socket by any of the peers so I'm not sure how ECONNRESET could be triggered.

@apapirovski
Copy link
Member Author

@lpinca I'm pretty sure the response event can trigger before the full \r\n\r\n bit makes it through, no? In general we tend to assume in our tests that chunks will make it through exactly as they're sent but it's not always true when the OS is busy.

@lpinca
Copy link
Member

lpinca commented Jun 9, 2018

@apapirovski you sure? afaik the 'response' event is emitted only after receiving \r\n\r\n?

@apapirovski
Copy link
Member Author

apapirovski commented Jun 9, 2018

@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
Copy link
Member

lpinca commented Jun 10, 2018

@apapirovski yes I was actually wondering if this was caused by a deeper bug. Thanks.
Anyway changes look good to me.

@addaleax
Copy link
Member

@addaleax addaleax added dont-land-on-v8.x author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 18, 2018
@maclover7
Copy link
Contributor

New CI since old one 404s now: https://ci.nodejs.org/job/node-test-pull-request/16078/

@targos
Copy link
Member

targos commented Aug 4, 2018

@maclover7
Copy link
Contributor

@apapirovski @addaleax I believe this should be ready to land, right? Just needs another CI run (since the old ones keep becoming stale)?

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

jasnell pushed a commit that referenced this pull request 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
Copy link
Member

jasnell commented Aug 12, 2018

Landed in 0aae34f

@jasnell jasnell closed this Aug 12, 2018
rvagg pushed a commit that referenced this pull request Aug 13, 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants