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

process: add .stillSynchronous #28941

Closed
wants to merge 1 commit into from
Closed

Conversation

ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Aug 2, 2019

This adds a process.stillSynchronous public property to check if the process is still in synchronous state (i.e. a sync script or startup phase of an async app/server) or not.

Could be used as an additional safeguard for #28439 (refs: #28439 (comment)), could be used to fence large deprecations like #22584 (the approach there was broken as far as I remember), also refs: nodejs/help#1740 (though this does not expose the tick id, only if we got past the first one or not).

Operates very similar to --trace-sync-io, ref: #28926.
Upd: switched to perf_hooks instead.

Docs will be added soon.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@ChALkeR ChALkeR added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 2, 2019
@ChALkeR ChALkeR requested a review from addaleax August 2, 2019 14:17
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 2, 2019
@ChALkeR ChALkeR requested a review from targos August 2, 2019 14:17
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Not to bikeshed too much, but how about a name like process.firstTick (although that gets confusing because process.nextTick() does not escape that – which we may also want to consider a bug) or process.firstEventLoopTurn? I think those might be clearer…

src/node_process_methods.cc Outdated Show resolved Hide resolved
src/env.h Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented Aug 2, 2019

I often have application logic that sets up across multiple ticks. Fetching info from a local server to setup the environment, etc, and I assume I'm not the only person who does that. Based on that, I really wouldn't want a random library I'm using to stop working because it assumes something about when the application's "setup" time is. We can still use this internally, but I don't think it should be exposed.

src/env-inl.h Outdated Show resolved Hide resolved
@ChALkeR ChALkeR added process Issues and PRs related to the process subsystem. and removed c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 2, 2019
@ChALkeR
Copy link
Member Author

ChALkeR commented Aug 2, 2019

@jasnell @addaleax I used perf_hooks now instead, it works nicely, thanks! No C++ part now.

@devsnek I don't think that this has much potential to be misused for such a purpose given the correct documentation (reasons: those changes you describe would be a semver-major for those modules, those modules would receive issues for misusing this API, also that is already possible in a simple (but less robust) way with setImmediate). But I think it could be useful for applications to analyze things and impose some safeguards on the app level.

@ChALkeR ChALkeR mentioned this pull request Aug 2, 2019
4 tasks
@mscdex
Copy link
Contributor

mscdex commented Aug 3, 2019

I agree stillSynchronous is a bit confusing, it makes me think it has something to do with I/O (e.g. stdout/stderr being in non-blocking mode or not). firstEventLoop or initialEventLoop would be better alternatives I think.

@BridgeAR
Copy link
Member

Ping @ChALkeR

@ChALkeR
Copy link
Member Author

ChALkeR commented Feb 18, 2020

@BridgeAR This got stuck on the naming. I'll just pick initialEventLoop for now.

@devsnek
Copy link
Member

devsnek commented Feb 18, 2020

I still don't understand the motivation for this feature (same concern as my above comment).

@addaleax
Copy link
Member

Hm … based on the discussion in #31765 – Maybe we want some kind of generic “event loop turn counter”?

I think the current state of the PR here would also include process.nextTick()s from the startup phase in the initialEventLoop phase, is that what we want? (In this case, “still synchronous” and “initial event loop” do refer to different things…)

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@jasnell
Copy link
Member

jasnell commented Jul 7, 2020

@ChALkeR ... still want to pursue this?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2020

Closing this because it has stalled. Feel free to reopen if this PR is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.

@github-actions github-actions bot closed this Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants