FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: remove SLAB_BUFFER_SIZE by apapirovski · Pull Request #21199 · nodejs/node · GitHub

/ node Public

tls: remove SLAB_BUFFER_SIZE - #21199

Closed
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-tls-remove-slab-buffer-size
ConversationCommits (1)ChecksFiles changed
Closed

tls: remove SLAB_BUFFER_SIZE#21199
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-tls-remove-slab-buffer-size

Conversation

apapirovski commented Jun 7, 2018

Copy link
Copy Markdown
Contributor

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

tls: remove SLAB_BUFFER_SIZE
e794198 
This constant has not been in use for many years now and the test
alongside it is invalid, as well as flaky.
apapirovski added the tls Issues and PRs related to the tls subsystem. label Jun 7, 2018

apapirovski commented Jun 7, 2018

Copy link
Copy Markdown
Contributor Author

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

addaleax approved these changes Jun 7, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Contributor 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 commented Jun 7, 2018

Copy link
Copy Markdown
Member

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

apapirovski commented Jun 7, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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 approved these changes Jun 8, 2018
View reviewed changes

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

@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 commented Jun 9, 2018

Copy link
Copy Markdown
Contributor 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 commented Jun 9, 2018

Copy link
Copy Markdown
Member

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

apapirovski commented Jun 9, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

Copy link
Copy Markdown
Member

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

addaleax commented Jul 18, 2018

Copy link
Copy Markdown
Member

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

addaleax added dont-land-on-v8.x author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Jul 18, 2018

maclover7 commented Jul 30, 2018

Copy link
Copy Markdown
Contributor

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

targos commented Aug 4, 2018

Copy link
Copy Markdown
Member

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

maclover7 commented Aug 11, 2018

Copy link
Copy Markdown
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 commented Aug 12, 2018

Copy link
Copy Markdown
Member

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

jasnell pushed a commit that referenced this pull request Aug 12, 2018
tls: remove SLAB_BUFFER_SIZE
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>

jasnell commented Aug 12, 2018

Copy link
Copy Markdown
Member

Landed in 0aae34f

jasnell closed this Aug 12, 2018
rvagg mentioned this pull request Aug 13, 2018
Merged
rvagg pushed a commit that referenced this pull request Aug 13, 2018
tls: remove SLAB_BUFFER_SIZE
7cb05cf 
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 was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
tls: remove SLAB_BUFFER_SIZE
2d1c185 
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
trivikr trivikr approved these changes
+1 more reviewer
ryzokuken ryzokuken approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

Back | FazBrowse Home | New Git URL