test: implement setproctitle for windows #14042
Conversation
refack
added
process
test/parallel/test-setproctitle.js
Outdated
| // The title shouldn't be too long; libuv's uv_set_process_title() out of | ||
| // security considerations no longer overwrites envp, only argv, so the | ||
| // maximum title length is possibly quite short. | ||
| let title = String(process.pid); | ||
| const title = String(Date.now()).substr(-4, 4); |
There was a problem hiding this comment.
Why this change? (to clarify: I’m sure there’s a reason, I just don’t see it?)
There was a problem hiding this comment.
I was checking what is actually the limit, and it turns out to be 4 chars, and it fails on my machine (when pid > 10000), so I picked a fairly random seed, and truncated to 4.
There was a problem hiding this comment.
Can you use a constant value then? I know this wasn’t the case before, but it’s not really a good idea to use random values in tests, where reproducibility matters (alternative: print the title to stderr, that should work as well)
There was a problem hiding this comment.
Ohhh. that's a good idea!
...
But it only give us node's "internal" view, while ps gives us how the external system sees the title
...
The issue with a constant value is that zombies will give us a false positive. We need a secret that only know to the testing process
There was a problem hiding this comment.
Oh, right. Yeah, in that case, print it out to stderr.
|
I could add some more comments, this is fairly black magic stuff... |
|
FWIW, the reason to be careful with constants (or something that can be equal by a chance in different runs) here: #12792 |
|
I'm totally not in love with the new code, if anybody has any idea how to improve it, please 🙏 |
test/parallel/test-setproctitle.js
Outdated
| const cmd = (() => { | ||
| if (common.isWindows) { | ||
| // For windows we need to spawn a new `window` so it will get the title. | ||
| // For that we need a `shell`, and to use `start` |
There was a problem hiding this comment.
Could do with a short comment here stating that changing the title on Windows changes the title of the Window, which may not be the node process. This add s bit of context as to why the test is different on Windows.
refack
force-pushed
the
complete-test-setproctitle
branch
from
47bcd52 to
d10d40d
Compare
| ); | ||
| const PSL = 'Powershell -NoProfile -ExecutionPolicy Unrestricted -Command'; | ||
| const winTitle = '$_.mainWindowTitle'; | ||
| return `${PSL} "ps | ? {${winTitle} -eq '${title}'} | % {${winTitle}}"`; |
There was a problem hiding this comment.
I think this is subject to race condition, where the PowerShell script may run before the spawned child process changes the title? Maybe we can use fork() instead of spawn() and use the IPC channel for sequencing? Would also have the benefit of not having to kill the child.
There was a problem hiding this comment.
I'll research that, also I think there is a guarantee that something (not sure what) has to finish before spawn returns.
Also AFAIK it's possible to establish an IPC via spawn
There was a problem hiding this comment.
P.S. I have seen inconsistant results but I attributed them to the child exiting too soon 🤔
There was a problem hiding this comment.
Again, with IPC we could get the child to wait around instead of arbitrary timeout.
There was a problem hiding this comment.
Can't use IPC because the script is run not in the child but in a grandchild created with START...
There was a problem hiding this comment.
Hm, is there any solution to this? Because without this solved it seems like there can not be any progress.
|
@refack this needs a rebase |
refack
force-pushed
the
complete-test-setproctitle
branch
from
d10d40d to
4b20201
Compare
|
@nodejs/testing PTAL |
|
@nodejs/platform-windows PTAL |
|
Ping @nodejs/testing @nodejs/platform-windows again. PTAL |
|
Closing due to no further progress. If someone thinks this should be reopened, please do so. |
Fixes: #14039
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test,process,windows