★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21199Nouvelle recherche | Portail wanayoo
Skip to content

/node

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

Closed
wants to merge 1 commit into from

Conversation

Reviewers

@jasnell jasnell

@addaleax addaleax

@cjihrig cjihrig

@joyeecheung joyeecheung

@ryzokuken ryzokuken

@trivikr trivikr

Assignees
No one assigned
Labels
author ready dont-land-on-v8.x tls
Projects
None yet
Milestone
No milestone
10 participants
@apapirovski @addaleax @lpinca @maclover7 @targos @jasnell @cjihrig @joyeecheung @ryzokuken @trivikr
@apapirovski
Copy link
Member

apapirovski commented Jun 7, 2018

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
@apapirovski
tls: remove SLAB_BUFFER_SIZE 
This constant has not been in use for many years now and the test
alongside it is invalid, as well as flaky.
Verified
This commit was signed with a verified signature.
@apapirovski apapirovski Anatoli Papirovski
GPG key ID: 614E2E1ABEB4B2C0 Learn about signing commits
Loading status checks…
e794198

@apapirovski apapirovski added the tls label Jun 7, 2018

@apapirovski

This comment has been minimized.

Sign in to view
Copy link
Member Author

apapirovski commented Jun 7, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15326/
@addaleax

addaleax approved these changes Jun 7, 2018
View changes

Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Member Author

apapirovski commented Jun 7, 2018

@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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member Author

apapirovski commented Jun 7, 2018 •
edited

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

trivikr approved these changes Jun 8, 2018
View changes

@joyeecheung

joyeecheung approved these changes Jun 8, 2018
View changes

Copy link
Member

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

ryzokuken approved these changes Jun 8, 2018
View changes

@cjihrig

cjihrig approved these changes Jun 8, 2018
View changes

@jasnell

jasnell approved these changes Jun 9, 2018
View changes

@lpinca

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member Author

apapirovski commented Jun 9, 2018

@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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member Author

apapirovski commented Jun 9, 2018 •
edited

@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

This comment has been minimized.

Sign in to view
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

This comment has been minimized.

Sign in to view
Copy link
Member

addaleax commented Jul 18, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/15936/

@addaleax addaleax added dont-land-on-v8.x author ready labels Jul 18, 2018

@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member

maclover7 commented Jul 30, 2018

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

This comment has been minimized.

Sign in to view
Copy link
Member

targos commented Aug 4, 2018

Resumed build: https://ci.nodejs.org/job/node-test-pull-request/16189/
@maclover7

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Aug 12, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16384/

jasnell added a commit that referenced this pull request Aug 12, 2018

@apapirovski @jasnell
tls: remove SLAB_BUFFER_SIZE 
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>
Loading status checks…
0aae34f
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

jasnell commented Aug 12, 2018

Landed in 0aae34f

@jasnell jasnell closed this Aug 12, 2018

rvagg added a commit that referenced this pull request Aug 13, 2018

@apapirovski @rvagg
tls: remove SLAB_BUFFER_SIZE 
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>
271cc12

@rvagg rvagg referenced this pull request Aug 13, 2018

Merged

Release proposal: v10.9.0 #22295

rvagg added a commit that referenced this pull request Aug 13, 2018

@apapirovski @rvagg
tls: remove SLAB_BUFFER_SIZE 
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>
7cb05cf

This was referenced Aug 16, 2018

Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.