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

lib: add late code injector #37503

Closed
wants to merge 3 commits into from

Conversation

RaisinTen
Copy link
Contributor

Fixes: #37440

@RaisinTen RaisinTen mentioned this pull request Feb 24, 2021
@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 24, 2021
@Trott
Copy link
Member

Trott commented Feb 24, 2021

Should this be documented? Does this need a test?

Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com>
@RaisinTen
Copy link
Contributor Author

@Trott yes, will surely add documentation + tests for this after setupLateCodeInjector works as expected.

@RaisinTen RaisinTen added the wip Issues and PRs that are still a work in progress. label Feb 24, 2021
@RaisinTen RaisinTen marked this pull request as draft February 24, 2021 15:22
@addaleax
Copy link
Member

In the current form, this should definitely be implemented in userland through NODE_OPTIONS=--require=... instead

@jasnell
Copy link
Member

jasnell commented Feb 24, 2021

Yeah, without more information about the specific set of use cases being targeted here, I have to agree with @addaleax

@gireeshpunathil
Copy link
Member

In the current form, this should definitely be implemented in userland through NODE_OPTIONS=--require=... instead

@addaleax - how does that work? with --require, the module should be present at startup right? my use case has node process predates the module. In theory, the module can be developed even after the production has started.

@devsnek
Copy link
Member

devsnek commented Feb 24, 2021

@gireeshpunathil at startup, load a small module loader script, instead of the main script:

'use strict';
process.on('SIGUSR1', () => {
  require(process.env.THE_MODULE);
  delete require.cache[require.resolve(process.env.THE_MODULE)];
});

@gireeshpunathil
Copy link
Member

@devsnek - makes sense, in the given premise of this PR ( there are certain preparation possible prior to the process launch). However, the original issue does not have such a premise. How do we run an arbitrary piece of code in a running process, where the process did not undergo any preparatory steps.

This PR is probably a middleground approach wherein the name of the module is set on the env , but probably that is coming from an implementation challenge of:

  • unable to export env data after startup
  • unable to pass data through signals

, not because we have that flexibility.

@mscdex
Copy link
Contributor

mscdex commented Feb 24, 2021

If you want to run arbitrary code after launch, why not start a tcp server in conjunction with repl and/or readline?

@addaleax
Copy link
Member

This PR is probably a middleground approach wherein the name of the module is set on the env

I wouldn't call this PR a midde ground, though, because it has the exact same set of requirements as what @devsnek and I would be suggesting. If you want a no-preparation-at-all scenario, then you'll need a different approach.

@gireeshpunathil
Copy link
Member

If you want to run arbitrary code after launch, why not start a tcp server in conjunction with repl and/or readline?

@mscdex - that needs the target process to be prepared for receiving an arbitrary code, right? Here is my requirement, restating:

Run an arbitrary piece of code in a running process, where the process did not undergo any preparatory steps.

@gireeshpunathil
Copy link
Member

If you want a no-preparation-at-all scenario, then you'll need a different approach.

@addaleax - yes agree. Please let @RaisinTen / me know if you have ideas on those lines.

@mscdex
Copy link
Contributor

mscdex commented Feb 25, 2021

@gireeshpunathil It sounds like what you're wanting is impossible then. Even with this PR it requires an environment variable on startup of the process?

@gireeshpunathil
Copy link
Member

Even with this PR it requires an environment variable on startup of the process?

@mscdex - agree, this PR assumes we are informed about a future attach.

@RaisinTen
Copy link
Contributor Author

Thanks for the reviews. Closing in favour of #37748.

@RaisinTen RaisinTen closed this Mar 15, 2021
@RaisinTen RaisinTen deleted the lib/add-late-code-injector branch March 15, 2021 12:18
indutny added a commit that referenced this pull request May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

late code injection
7 participants