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

fix: exit handler error on login #144

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

milaninfy
Copy link
Contributor

@milaninfy milaninfy commented Jul 30, 2024

One of the possible cause that the exit handler never called happens is when this timer doesn't keep event loop active when ref is set to false. Removing set it to true by default.
timers/promises/setTimeout
Fixes: npm/cli#7612

@milaninfy milaninfy marked this pull request as ready for review July 31, 2024 17:04
@milaninfy milaninfy requested a review from a team as a code owner July 31, 2024 17:04
@wraithgar
Copy link
Member

Yeah iirc that ref: false was intentional because we don't want to keep the event loop open if the retry isn't needed. We'll need to take a deeper look at this retry logic, it was refactored recently to use abortsignal.

@reggi
Copy link
Contributor

reggi commented Sep 20, 2024

I think this makes sense I tested it locally and it fixes the npm login --no-progress "hit enter" bug.

I think if we run this timeout we should have it be apart of nodes event loop.

Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

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

ref:false is causing the timer to not abide within the node event loop, and is causing the process to exit early.

@reggi reggi merged commit 4ea3f70 into npm:main Sep 20, 2024
19 checks passed
@github-actions github-actions bot mentioned this pull request Sep 20, 2024
reggi pushed a commit that referenced this pull request Sep 26, 2024
🤖 I have created a release *beep* *boop*
---


##
[11.0.0](v10.0.0...v11.0.0)
(2024-09-26)
### ⚠️ BREAKING CHANGES
* `npm-profile` now supports node `^18.17.0 || >=20.5.0`
### Bug Fixes
*
[`a0ea10b`](a0ea10b)
[#152](#152) align to npm 10 node
engine range (@reggi)
*
[`4ea3f70`](4ea3f70)
[#144](#144) exit handler error
on login (#144) (@milaninfy)
### Dependencies
*
[`66bcc40`](66bcc40)
[#152](#152) `proc-log@5.0.0`
### Chores
*
[`8ac1fdb`](8ac1fdb)
[#152](#152) run
template-oss-apply (@reggi)
*
[`1fdff2e`](1fdff2e)
[#146](#146) bump
@npmcli/eslint-config from 4.0.5 to 5.0.0 (@dependabot[bot])
*
[`5b3ebbc`](5b3ebbc)
[#134](#134) bump
@npmcli/template-oss to 4.22.0 (@lukekarrys)
*
[`6b4558f`](6b4558f)
[#147](#147) postinstall for
dependabot template-oss PR (@hashtagchris)
*
[`c644e89`](c644e89)
[#147](#147) bump
@npmcli/template-oss from 4.23.1 to 4.23.3 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] npm login - Exit handler never called!
3 participants