watch: fix --env-file-if-exists crashing on linux if the file is missing#61870
watch: fix --env-file-if-exists crashing on linux if the file is missing#61870efekrskl wants to merge 7 commits intonodejs:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61870 +/- ##
==========================================
+ Coverage 89.72% 89.76% +0.04%
==========================================
Files 675 674 -1
Lines 204806 204903 +97
Branches 39355 39386 +31
==========================================
+ Hits 183761 183940 +179
+ Misses 13330 13234 -96
- Partials 7715 7729 +14
🚀 New features to boost your workflow:
|
lib/internal/main/watch_mode.js
Outdated
| if (existsSync(resolvedPath)) { | ||
| watcher.filterFile(resolvedPath); |
There was a problem hiding this comment.
There's a TOCTOU race condition here, the file could be deleted between the existSync call and the watcher.filterFile. Could we use something reliable?
There was a problem hiding this comment.
Ah, good point. I've changed the implementation a bit, instead of using existsSync, now we would try/catch and swallow the errors. This seems to be the approach in some other places. WDYT?
lib/internal/main/watch_mode.js
Outdated
| try { | ||
| watcher.filterFile(resolve(file)); | ||
| } catch (error) { | ||
| if (error?.code !== 'ENOENT' && error?.code !== 'ENOTDIR') { | ||
| throw error; | ||
| } | ||
| // Failed watching the file, ignore | ||
| } |
There was a problem hiding this comment.
This is a really unperformant way of doing this. Creating, throwing and catching errors is extremely costly. Ideally just like fs.statSync we should have a way of not throwing ENOENT errors from this method.
|
|
||
| if (throwIfNoEntry != null) { | ||
| validateBoolean(throwIfNoEntry, 'options.throwIfNoEntry'); | ||
| } |
There was a problem hiding this comment.
Can you just add a default value, which will simplify tracing?
| } | |
| } else { | |
| throwIfNoEntry = true; | |
| } |
| } | ||
| } catch (error) { | ||
| if (error.code === 'ENOENT') { | ||
| if (this.#options.throwIfNoEntry !== false && error.code === 'ENOENT') { |
There was a problem hiding this comment.
If you add a default value, this will be less error prune.
| if (this.#options.throwIfNoEntry !== false && error.code === 'ENOENT') { | |
| if (!this.#options.throwIfNoEntry && error.code === 'ENOENT') { |
There was a problem hiding this comment.
I've added the default, but I think using ! instead of explicitly checking for false would be against our intention, no?
lib/internal/main/watch_mode.js
Outdated
| ArrayPrototypeForEach(kEnvFiles, (file) => watcher.filterFile(resolve(file))); | ||
| } | ||
| if (kOptionalEnvFiles.length > 0) { | ||
| ArrayPrototypeForEach(kOptionalEnvFiles, (file) => watcher.filterFile(resolve(file), undefined, true)); |
There was a problem hiding this comment.
Since you didn't use options like an object, it is now extremely hard to trace this method. If you prefer to use boolean value for passing the throwIfNoEntry, which might be something you prefer (or not), I recommend adding a small comment next to the parameter to make things a lot more maintainable (and readable) for others.
| ArrayPrototypeForEach(kOptionalEnvFiles, (file) => watcher.filterFile(resolve(file), undefined, true)); | |
| ArrayPrototypeForEach(kOptionalEnvFiles, (file) => watcher.filterFile(resolve(file), undefined, true /* throwIfNoEntry */)); |
There was a problem hiding this comment.
Ah, ok, thanks. In that case I think it'd make more sense to have an options object. Done
|
@anonrig I've addressed all comments. I've also documented the change in Also, thanks a lot for the quick and detailed feedback! |
5 participants
Fixes #57040
--env-file-if-exists combined with --watch seems to crash in if the .env file is missing (in Linux), this PR aims to fix that.