Skip to content

http: remove unused hasItems() from freelist - #30744

Merged
Trott merged 0 commit into
nodejs:masterfrom
Trott:rm-hasitems
Dec 3, 2019
Merged

http: remove unused hasItems() from freelist#30744
Trott merged 0 commit into
nodejs:masterfrom
Trott:rm-hasitems

Conversation

@Trott

@Trott Trott commented Dec 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

Remove the hasItems() method from freelist module as it is unused
internally.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@Trott Trott added http Issues and PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 1, 2019
@Trott

Trott commented Dec 1, 2019

Copy link
Copy Markdown
Member Author

Marking semver-major out of an abundance of caution as the API is exposed in one place:

$ node -p "require('_http_common').parsers.hasItems()"
false
$

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@Trott

Trott commented Dec 1, 2019

Copy link
Copy Markdown
Member Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2099/

@Trott
Trott requested a review from addaleax December 1, 2019 01:08
@Trott Trott added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Dec 1, 2019
@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2019 •
edited by Trott
Loading

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27241/ ✔️

@Trott

Trott commented Dec 2, 2019

Copy link
Copy Markdown
Member Author

CITGM on master for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2104/

(I'm seeing more commander failures than I expected on the CITGM for this PR. But that seems unrelated. So, running against master to see how it does there.)

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

And...CITGM this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2105/

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

The commander results for this PR continue to be puzzling to me. One more CITGM against master to see if it shows up there or not: https://ci.nodejs.org/view/All/job/citgm-smoker/2108/

@BridgeAR

BridgeAR commented Dec 3, 2019

Copy link
Copy Markdown
Member

@Trott the commander results are also on v13, so it's definitely independent from this PR.

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

@Trott the commander results are also on v13, so it's definitely independent from this PR.

Not just the fact that commander is failing, but that it fails the way it does in this PR?

On master, commander fails in two or three places for a few different reasons. Here, it fails in six places each time, and every time it's the same way, with a 5000ms timeout being exceeded (probably due to either a race condition or an async event not running at all).

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

@Trott the commander results are also on v13, so it's definitely independent from this PR.

Not just the fact that commander is failing, but that it fails the way it does in this PR?

On master, commander fails in two or three places for a few different reasons. Here, it fails in six places each time, and every time it's the same way, with a 5000ms timeout being exceeded (probably due to either a race condition or an async event not running at all).

Just checked for myself, and yeah, they show up on 13.2.0 CITGM, but a little less frequently. They've have to be unrelated. I'm being super-cautious though. Sorry for all the notifications.

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

One hopefully-last CITGM against this PR: https://ci.nodejs.org/job/citgm-smoker/2109/

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

OK, the CITGM commander issue showed up on master too so I think this is good to land. I'm still going to let this one last CITGM run. I'm still bugged that it seems to occur about twice as often with this change, which again, makes no sense, but stranger stuff has happened....

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

(Also, it's happening but only very infrequently for me locally on master.)

@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

OK, the CITGM commander issue showed up on master too so I think this is good to land. I'm still going to let this one last CITGM run. I'm still bugged that it seems to occur about twice as often with this change, which again, makes no sense, but stranger stuff has happened....

Last CITGM run with this PR, the timeout happened just twice, totally in line with master branch CITGM. Landing!

@Trott Trott closed this Dec 3, 2019
@Trott
Trott merged commit ff2ed3e into nodejs:master Dec 3, 2019
@Trott

Trott commented Dec 3, 2019

Copy link
Copy Markdown
Member Author

Landed in ff2ed3e

@Trott
Trott deleted the rm-hasitems branch December 3, 2019 23:48
@BethGriggs BethGriggs mentioned this pull request Mar 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@addaleax addaleax addaleax approved these changes
@lpinca lpinca lpinca approved these changes
@cjihrig cjihrig cjihrig approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
@trivikr trivikr trivikr approved these changes
+1 more reviewer
@starkwang starkwang starkwang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. http Issues and PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@Trott @nodejs-github-bot @BridgeAR @jasnell @addaleax @lpinca @cjihrig @starkwang @trivikr