Skip to content
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.

Sign up for GitHub

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

Jump to bottom

os: ReadDir fails on file systems without File ID support on Windows [1.21 backport] #61910

Closed
gopherbot opened this issue Aug 9, 2023 · 7 comments
Closed

os: ReadDir fails on file systems without File ID support on Windows [1.21 backport] #61910

gopherbot opened this issue Aug 9, 2023 · 7 comments
Labels
CherryPickApproved Used during the release process for point releases OS-Windows release-blocker
Milestone
Go1.21.1

Comments

@gopherbot
Copy link

@bcmills requested issue #61907 to be considered for backport to the next 1.21 minor release.

@gopherbot, please backport to Go 1.21. This is a significant regression in ReadDir.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Aug 9, 2023
@gopherbot gopherbot mentioned this issue Aug 9, 2023
Closed
@gopherbot gopherbot added this to the Go1.21.1 milestone Aug 9, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/518196 mentions this issue: os: support file systems without file IDs when reading directories on windows

@hajimehoshi hajimehoshi changed the title os: ReadDir fails for UNC paths shared with Windows using Parallels [1.21 backport] os: ReadDir fails on file systems without File ID support on Windows [1.21 backport] Aug 10, 2023
@dmitshur
Copy link
Contributor

This is a significant regression in ReadDir.

@bcmills Can you please provide a more detailed rationale for this backport? Thanks.

@dmitshur
Copy link
Contributor

dmitshur commented Aug 16, 2023 •
edited

@aclements suggests a possible alternative path of reverting the optimization (if viable) and then applying the stronger fix for Go 1.22 only.

@dmitshur dmitshur added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 16, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/520156 mentions this issue: [release-branch.go1.21] Revert "os: use handle based APIs to read directories on windows"

@bcmills
Copy link
Contributor

bcmills commented Aug 16, 2023

Can you please provide a more detailed rationale for this backport? Thanks.

Without some kind of backport fix, calling Readdir or ReadDir fails for an os.File representing a directory in certain filesystems (notably SMB shares). That breaks a number of tools, including cmd/go in some cases (see the original report in #61907).

@bcmills
Copy link
Contributor

bcmills commented Aug 16, 2023

@aclements Suggests a possible alternative path of reverting the optimization (if viable)

It looks like it is feasible to revert: https://go.dev/cl/520156 required a couple of non-trivial merge resolutions but overall seems to apply fairly cleanly.

@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Aug 16, 2023
@cagedmantis cagedmantis added the CherryPickApproved Used during the release process for point releases label Aug 23, 2023
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Aug 23, 2023
@cagedmantis cagedmantis mentioned this issue Aug 23, 2023
Closed
gopherbot pushed a commit that referenced this issue Aug 23, 2023
@cagedmantis
[release-branch.go1.21] Revert "os: use handle based APIs to read dir…
7c97cc7 
…ectories on windows"

This reverts CL 452995.

Reason for revert: caused os.File.ReadDir to fail on
filesystems that do not support FILE_ID_BOTH_DIR_INFO.

This is an alternative to a fix-forward change in CL 518196.
Since the original change was mostly a performance improvement,
reverting to the previous implementation seems less risky than
backporting a larger fix.

Fixes #61910
Fixes #61964

Change-Id: I60f1602b9eb6ea353e7eb23429f19f1ffa16b394
Reviewed-on: https://go-review.googlesource.com/c/go/+/520156
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
@gopherbot
Copy link
Author

Closed by merging 7c97cc7 to release-branch.go1.21.

awly pushed a commit to tailscale/go that referenced this issue Feb 7, 2024
@awly
[release-branch.go1.21] Revert "os: use handle based APIs to read dir…
8c3391a 
…ectories on windows"

This reverts CL 452995.

Reason for revert: caused os.File.ReadDir to fail on
filesystems that do not support FILE_ID_BOTH_DIR_INFO.

This is an alternative to a fix-forward change in CL 518196.
Since the original change was mostly a performance improvement,
reverting to the previous implementation seems less risky than
backporting a larger fix.

Fixes golang#61910
Fixes golang#61964

Change-Id: I60f1602b9eb6ea353e7eb23429f19f1ffa16b394
Reviewed-on: https://go-review.googlesource.com/c/go/+/520156
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
JanDeDobbeleer pushed a commit to JanDeDobbeleer/go that referenced this issue Feb 12, 2024
@JanDeDobbeleer
[release-branch.go1.21] Revert "os: use handle based APIs to read dir…
1fc27a3 
…ectories on windows"

This reverts CL 452995.

Reason for revert: caused os.File.ReadDir to fail on
filesystems that do not support FILE_ID_BOTH_DIR_INFO.

This is an alternative to a fix-forward change in CL 518196.
Since the original change was mostly a performance improvement,
reverting to the previous implementation seems less risky than
backporting a larger fix.

Fixes golang#61910
Fixes golang#61964

Change-Id: I60f1602b9eb6ea353e7eb23429f19f1ffa16b394
Reviewed-on: https://go-review.googlesource.com/c/go/+/520156
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases OS-Windows release-blocker
Projects
None yet
Milestone
Go1.21.1
Development

No branches or pull requests
5 participants
@cagedmantis @dmitshur @qmuntal @bcmills @gopherbot