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

http: remove unused hasItems() from freelist #30744

Merged
merged 0 commits into from
Dec 3, 2019
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 1, 2019

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

@Trott
Copy link
Member Author

Trott commented Dec 1, 2019

@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
Collaborator

nodejs-github-bot commented Dec 1, 2019

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

Trott commented Dec 3, 2019

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

Trott commented Dec 3, 2019

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

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

Trott commented Dec 3, 2019

Landed in ff2ed3e

@Trott Trott deleted the rm-hasitems branch December 3, 2019 23:48
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. 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
Development

Successfully merging this pull request may close these issues.

9 participants