Skip to content

http: remove unused hasItems() from freelist#30744

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

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

Conversation

@Trott
Copy link
Copy Markdown
Member

@Trott Trott commented Dec 1, 2019 •
edited
Loading

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 or 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
Copy link
Copy Markdown
Member Author

Trott commented Dec 1, 2019

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
Copy link
Copy Markdown
Member Author

Trott commented Dec 1, 2019

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 that have at least one approval, no pending requests for changes, and a CI started. label Dec 1, 2019
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

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

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

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 2, 2019

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
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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

BridgeAR commented Dec 3, 2019

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

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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

Trott commented Dec 3, 2019

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

Trott commented Dec 3, 2019

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

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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

@Trott
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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
Copy link
Copy Markdown
Member Author

Trott commented Dec 3, 2019

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 that have at least one approval, no pending requests for changes, and a CI started. http Issues or 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