Skip to content

child_process: Fix regression on Windows for FAT filesystems #16492

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

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

squeek502
Copy link
Collaborator

@squeek502 squeek502 commented Jul 23, 2023

This fixes a regression caused by #13993

As an optimization, the first call to NtQueryDirectoryFile would only ask for a single result and assume that if the result returned did not match the app_name exactly, then the unappended app_name did not exist. However, this relied on the assumption that the unappended app_name would always be returned first, but that only seems to be the case on NTFS. On FAT filesystems, the order of returned files can be different, which meant that it could assume the unappended file doesn't exist when it actually does.

This commit fixes that by fully iterating the wildcard matches via NtQueryDirectoryFile and taking note of any unappended/PATHEXT-appended filenames it finds. In practice, this strategy does not introduce a speed regression compared to the previous (buggy) implementation.

Benchmark 1 (10 runs): winpathbench-master.exe
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           508ms ± 4.08ms     502ms …  517ms          1 (10%)        0%
  peak_rss           3.62MB ± 2.76KB    3.62MB … 3.63MB          0 ( 0%)        0%
Benchmark 2 (10 runs): winpathbench-fat32-fix.exe
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           500ms ± 21.4ms     480ms …  535ms          0 ( 0%)          -  1.5% ±  2.8%
  peak_rss           3.62MB ± 2.76KB    3.62MB … 3.63MB          0 ( 0%)          -  0.0% ±  0.1%

(same benchmark as #13993, but different PATH environment variable)


Partially addresses #16374 (it fixes zig build on FAT32 when no zig-cache is present, but error.Unexpected NTSTATUS=0xc0000035 still happens during zig build once zig-cache is created)

Before (where E:\ is a FAT32 filesystem):

E:\zigtest> zig build
error: FileNotFound

After this commit:

E:\zigtest> zig build

E:\zigtest> ".\zig-out\bin\zigtest.exe"
All your codebase are belong to us.
Run `zig build test` to run the tests.

Note: there's a fairly comprehensive standalone test for Windows spawn behavior, so as long as the tests pass we can have some confidence that this won't regress the spawn behavior:

https://github.com/ziglang/zig/blob/master/test/standalone/windows_spawn/main.zig

This fixes a regression caused by ziglang#13993

As an optimization, the first call to `NtQueryDirectoryFile` would only ask for a single result and assume that if the result returned did not match the app_name exactly, then the unappended app_name did not exist. However, this relied on the assumption that the unappended app_name would always be returned first, but that only seems to be the case on NTFS. On FAT filesystems, the order of returned files can be different, which meant that it could assume the unappended file doesn't exist when it actually does.

This commit fixes that by fully iterating the wildcard matches via `NtQueryDirectoryFile` and taking note of any unappended/PATHEXT-appended filenames it finds. In practice, this strategy does not introduce a speed regression compared to the previous (buggy) implementation.

Benchmark 1 (10 runs): winpathbench-master.exe
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           508ms ± 4.08ms     502ms …  517ms          1 (10%)        0%
  peak_rss           3.62MB ± 2.76KB    3.62MB … 3.63MB          0 ( 0%)        0%
Benchmark 2 (10 runs): winpathbench-fat32-fix.exe
  measurement          mean ± σ            min … max           outliers         delta
  wall_time           500ms ± 21.4ms     480ms …  535ms          0 ( 0%)          -  1.5% ±  2.8%
  peak_rss           3.62MB ± 2.76KB    3.62MB … 3.63MB          0 ( 0%)          -  0.0% ±  0.1%

---

Partially addresses ziglang#16374 (it fixes `zig build` on FAT32 when no `zig-cache` is present)
@andrewrk
Copy link
Member

Thank you @squeek502. Excellent work, as usual, and I really appreciate your detailed writeups.

@andrewrk andrewrk merged commit 3f7166e into ziglang:master Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants