fix(node): Pass NPM_PROCESS_STATE to subprocesses via temp file instead of env var #25896
Conversation
cli/args/mod.rs
Outdated
| // seek to beginning, this file might have been read before | ||
| file.seek(std::io::SeekFrom::Start(0)).unwrap(); |
There was a problem hiding this comment.
Would this actually ever happen? It's a unique fd per processs, right? Maybe we shouldn't bother seeking?
There was a problem hiding this comment.
When the child process inherits the fd, the seek position is shared as well (I spent like an hour debugging why the child was reading an empty string and this was why).
So you have to seek somewhere, you either seek immediately after writing the temp file, or seek before reading. Seemed more reliable to just do it before reading.
It does get reused also with lifecycle scripts - there we make a single tempfile and then it gets inherited by all of the lifecycle script subprocesses
There was a problem hiding this comment.
Ah, I see. Maybe it would be good to say in a comment that the seek is necessary because of the write.
It does get reused also with lifecycle scripts - there we make a single tempfile and then it gets inherited by all of the lifecycle script subprocesses
Is the position synced between the child processes? If so, there might be a race condition here.
There was a problem hiding this comment.
It is synced between the child processes, but we don't actually run multiple tasks concurrently (though maybe we should, between packages at least?), so it should be safe. I'll add a comment to that effect
cli/args/mod.rs
Outdated
|
|
||
| // static NPM_PROCESS_STATE: Lazy<Option<NpmProcessState>> = Lazy::new(|| None); |
cli/args/mod.rs
Outdated
| file.seek(std::io::SeekFrom::Start(0)).unwrap(); | ||
| file.read_to_end(&mut buf).unwrap(); |
There was a problem hiding this comment.
This will cause panics if anything goes wrong. Maybe we should instead log some error to help ourselves debugging in the future?
| let temp_file_fd = | ||
| deno_runtime::ops::process::npm_process_state_tempfile( | ||
| process_state.as_bytes(), | ||
| )?; |
runtime/ops/process.rs
Outdated
| let process_state = provider.get_npm_process_state(); | ||
| let fd = npm_process_state_tempfile(process_state.as_bytes())?; | ||
| args.env.push(( | ||
| "DENO_DONT_USE_INTERNAL_NODE_COMPAT_STATE_FD".to_string(), |
There was a problem hiding this comment.
Nitpick: maybe make it a static and import it in the CLI?
Fixes #25401. Fixes #25841. Fixes #25891.
I don't love where things are located module-wise, if anyone has strong opinions on where to put some of this code I'll shuffle things around.