Small fixes in FileSystemEnumerator for Windows.#126544
Small fixes in FileSystemEnumerator for Windows.#126544teo-tsirpanis wants to merge 6 commits intodotnet:mainfrom
FileSystemEnumerator for Windows.#126544Conversation
…temEnumerator buffer Agent-Logs-Url: https://github.com/teo-tsirpanis/dotnet-runtime/sessions/233c35f2-6fa4-4409-b093-93f4a80a0598 Co-authored-by: teo-tsirpanis <12659251+teo-tsirpanis@users.noreply.github.com>
… cast Agent-Logs-Url: https://github.com/teo-tsirpanis/dotnet-runtime/sessions/c20f57ce-bd4d-4583-90a1-c799d80ee39e Co-authored-by: teo-tsirpanis <12659251+teo-tsirpanis@users.noreply.github.com>
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows-specific implementation details in FileSystemEnumerator<TResult> to ensure the intended OS behaviors are actually applied and to make the NT query buffer’s alignment requirement explicit.
Changes:
- Replace
using (default(DisableMediaInsertionPrompt))withusing (DisableMediaInsertionPrompt.Create())so the media insertion prompt suppression is actually enabled. - Allocate the directory query buffer via
NativeMemory.AlignedAlloc(..., sizeof(ulong))and free it viaNativeMemory.AlignedFree, switching_bufferfromIntPtrtovoid*. - Update the
NtQueryDirectoryFileinterop signature to acceptvoid*forFileInformation, matching the updated buffer type.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/libraries/System.Private.CoreLib/src/System/IO/Enumeration/FileSystemEnumerator.Windows.cs |
Uses the correct prompt-suppression factory and switches the query buffer to explicitly aligned native allocation. |
src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtQueryDirectoryFile.cs |
Updates the P/Invoke signature to take a raw pointer for the output buffer. |
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
@JeremyKuhne PTAL
Thank you for your contribution @teo-tsirpanis !
| // We'll only suppress the media insertion prompt on the topmost directory as that is the | ||
| // most likely scenario and we don't want to take the perf hit for large enumerations. | ||
| // (We weren't consistent with how we handled this historically.) | ||
| using (default(DisableMediaInsertionPrompt)) |
There was a problem hiding this comment.
It seems like we were doing this on purpose. I will defer to @JeremyKuhne for review.
There was a problem hiding this comment.
This has been broken for nearly 10 years I'd recommend just removing it- it clearly isn't impacting anyone.
JeremyKuhne
left a comment
JeremyKuhne
left a comment
There was a problem hiding this comment.
Generally looks fine, see my comment on removing the broken suppression.
| // We'll only suppress the media insertion prompt on the topmost directory as that is the | ||
| // most likely scenario and we don't want to take the perf hit for large enumerations. | ||
| // (We weren't consistent with how we handled this historically.) | ||
| using (default(DisableMediaInsertionPrompt)) |
There was a problem hiding this comment.
This has been broken for nearly 10 years I'd recommend just removing it- it clearly isn't impacting anyone.
| // We need to initialize the directory handle up front to ensure | ||
| // we immediately throw IO exceptions for missing directory/etc. | ||
| _directoryHandle = CreateDirectoryHandle(_rootDirectory); | ||
| if (_directoryHandle == IntPtr.Zero) | ||
| _lastEntryFound = true; |
There was a problem hiding this comment.
Init() used to attempt to suppress the Windows removable-media insertion prompt when opening the top-level directory handle, but the PR removes the DisableMediaInsertionPrompt scope entirely. This doesn't match the PR description (“replace default(...) with Create()”) and leaves CreateDirectoryHandle(_rootDirectory) unprotected from prompting. Please wrap the directory-handle creation in using (DisableMediaInsertionPrompt.Create()) (keeping the existing intent of only doing this for the topmost directory to avoid perf impact).
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
LGTM, thank you @teo-tsirpanis !
Note
This PR was created with the assistance of GitHub Copilot.
Replaced ineffective use ofRemoved, after PR feedback.default(DisableMediaInsertionPrompt), withDisableMediaInsertionPrompt.Create()._bufferwas updated to be backed byNativeMemory.AlignedAllocinstead ofMarshal.AllocHGlobal. This makes the alignment requirement explicit and might increase performance.