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.

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

os: document concurrency properties for File methods #56043

Open
bcmills opened this issue Oct 4, 2022 · 5 comments
Open

os: document concurrency properties for File methods #56043

bcmills opened this issue Oct 4, 2022 · 5 comments
Labels
Documentation ExpertNeeded NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@bcmills
Copy link
Member

bcmills commented Oct 4, 2022

What version of Go are you using (go version)?

https://pkg.go.dev/os@go1.19.1

Does this issue reproduce with the latest release?

Yes.

What did you do?

For #50436, I'm attempting to unblock reads on a *File returned by os.Pipe while it is being read concurrently by a user-controlled goroutine. The user-controlled goroutine may legitimately call the Close method, and may expect to be able to access other methods (such as SetDeadline) via type-assertion.

What did you expect to see?

Given #6270, #7970, #9307, #17647 and https://go.dev/cl/65490, I expected the documentation for `*os.File to describe which methods are safe to invoke concurrently and under what conditions.

What did you see instead?

The only mention of concurrency I could find in https://pkg.go.dev/os@go1.19.1 says this:

Note: The maximum number of concurrent operations on a File may be limited by the OS or the system. The number should be high, but exceeding it may degrade performance or cause other issues.

That seems to imply that at least some of the File methods may be invoked concurrently, but isn't explicit about which ones.

I see unsynchronized writes in the Close implementation on both unix and plan9 (but maybe not windows?); it's not obvious to me which other methods are or aren't safe.

(CC @rsc @ianlancetaylor @robpike from previous *File race conditions.)

@bcmills bcmills added ExpertNeeded Documentation NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Oct 4, 2022
@bcmills bcmills added this to the Backlog milestone Oct 4, 2022
@robpike
Copy link
Contributor

robpike commented Oct 4, 2022

Unless we're willing to put locks everywhere, I'm not sure what we can say beyond saying to read the documents for the operating system.

@gopherbot
Copy link

gopherbot commented Oct 4, 2022

Change https://go.dev/cl/438347 mentions this issue: os: add test that concurrent calls to Close on a pipe are OK

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Oct 4, 2022

As it happens, we actually already have all the required locks for regular files or pipes, on all systems other than Plan 9. I've already sent CL 438347 to fix Plan 9.

We have the locks because we need them anyhow for the runtime poller.

The current exception is for directories. For directories os files have a dirInfo field. There are no locks around access to that field.

@gopherbot
Copy link

gopherbot commented Oct 5, 2022

Change https://go.dev/cl/439195 mentions this issue: os/exec: remove protection against a duplicate Close on StdinPipe

@gopherbot
Copy link

gopherbot commented Oct 6, 2022

Change https://go.dev/cl/439595 mentions this issue: os: ensure that concurrent I/O and Close on a pipe are OK

gopherbot pushed a commit that referenced this issue Oct 8, 2022
This permits us to safely support concurrent access to files on Plan 9.
Concurrent access was already safe on other systems.

This does introduce a change: if one goroutine calls a blocking read
on a pipe, and another goroutine closes the pipe, then before this CL
the close would occur. Now the close will be delayed until the blocking
read completes.

Also add tests that concurrent I/O and Close on a pipe are OK.

For #50436
For #56043

Change-Id: I969c869ea3b8c5c2f2ef319e441a56a3c64e7bf5
Reviewed-on: https://go-review.googlesource.com/c/go/+/438347
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: David du Colombier <0intro@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
gopherbot pushed a commit that referenced this issue Oct 11, 2022
As of CL 438347, multiple concurrents calls to Close should be safe.

This removes some indirection and may also make some programs that use
type-assertions marginally more efficient. For example, if a program
calls (*exec.Cmd).StdinPipe to obtain a pipe and then sets that as the
Stdout of another command, that program will now allow the second
command to inherit the file descriptor directly instead of copying
everything through a goroutine.

This will also cause calls to Close after the first to return an error
wrapping os.ErrClosed instead of nil. However, it seems unlikely that
programs will depend on that error behavior: if a program is calling
Write in a loop followed by Close, then if a racing Close occurs it is
likely that the Write would have already reported an error. (The only
programs likely to notice a change are those that call Close — without
Write! — after a call to Wait.)

Updates #56043.
Updates #9307.
Updates #6270.

Change-Id: Iec734b23acefcc7e7ad0c8bc720085bc45988efb
Reviewed-on: https://go-review.googlesource.com/c/go/+/439195
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
This permits us to safely support concurrent access to files on Plan 9.
Concurrent access was already safe on other systems.

This does introduce a change: if one goroutine calls a blocking read
on a pipe, and another goroutine closes the pipe, then before this CL
the close would occur. Now the close will be delayed until the blocking
read completes.

Also add tests that concurrent I/O and Close on a pipe are OK.

For golang#50436
For golang#56043

Change-Id: I969c869ea3b8c5c2f2ef319e441a56a3c64e7bf5
Reviewed-on: https://go-review.googlesource.com/c/go/+/438347
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: David du Colombier <0intro@gmail.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Rob Pike <r@golang.org>
romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
As of CL 438347, multiple concurrents calls to Close should be safe.

This removes some indirection and may also make some programs that use
type-assertions marginally more efficient. For example, if a program
calls (*exec.Cmd).StdinPipe to obtain a pipe and then sets that as the
Stdout of another command, that program will now allow the second
command to inherit the file descriptor directly instead of copying
everything through a goroutine.

This will also cause calls to Close after the first to return an error
wrapping os.ErrClosed instead of nil. However, it seems unlikely that
programs will depend on that error behavior: if a program is calling
Write in a loop followed by Close, then if a racing Close occurs it is
likely that the Write would have already reported an error. (The only
programs likely to notice a change are those that call Close — without
Write! — after a call to Wait.)

Updates golang#56043.
Updates golang#9307.
Updates golang#6270.

Change-Id: Iec734b23acefcc7e7ad0c8bc720085bc45988efb
Reviewed-on: https://go-review.googlesource.com/c/go/+/439195
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation ExpertNeeded NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants