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

cmd/go/internal/modload: inconsistent locking of go.mod file #38719

Closed
millerresearch opened this issue Apr 28, 2020 · 6 comments
Closed

cmd/go/internal/modload: inconsistent locking of go.mod file #38719

millerresearch opened this issue Apr 28, 2020 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@millerresearch
Copy link
Contributor

millerresearch commented Apr 28, 2020

In most instances where a go.mod file is read or written, it's locked using cmd/go/internal/lockedfile. But function (*mvsReqs).required in modload/mvs.go just reads it using ioutil.ReadFile. This seems a lapse from the usual discipline of locking: if a file needs protection from data races between simultaneous operations, shouldn't all accesses be performed under the same lock?

In practice the inconsistent use of locking go.mod causes a problem in Plan 9, and potentially in other operating systems which have no flock system call. Plan 9 implements lockedfile by setting os.modeExclusive permissions on the file when it is first locked, and repeatedly retrying when simultaneous attempts at lockedfile.OpenFile are excluded. This means that once a file has been locked, any subsequent simultaneous opens without using lockedfile will not be retried, and simply fail.

This has been causing frequent failures in some of the subrepo tests, for example this one where we see this error message because go.mod is being parsed by more than one goroutine at once:

/boot/workdir/gopath/src/golang.org/x/text/message/format.go:12:2: could not import golang.org/x/text/internal/format (go/build: go list golang.org/x/text/internal/format: exit status: 'go 82528: 1'
go: golang.org/x/text@v0.0.0-00010101000000-000000000000: parsing /boot/workdir/gopath/src/golang.org/x/text/go.mod: open /boot/workdir/gopath/src/golang.org/x/text/go.mod: '/boot/workdir/gopath/src/golang.org/x/text/go.mod' exclusive lock

You could say that the Plan 9 implementation of lockedfile is the problem. Certainly its semantics is different from the linux version, but it's hard to guess from go doc whether the locking is meant to be advisory or mandatory. (Or is there another specification somewhere?)

My suggestion would be to use lockedfile.Read in place of ioutil.ReadFile whenever parsing go.mod. But without a deep knowledge of how the module mechanism works, I can't say whether this might cause other problems. Maybe @bcmills or @rsc can advise?

@jayconrod
Copy link
Contributor

@bcmills can probably correct me on this, but I believe lockedfile.Read is meant to be advisory: it's meant to be used in situations where multiple go processes might read and write the go.mod file concurrently. ioutil.ReadFile is used in situations where go.mod is not expected to be written. In particular, (*mvsReqs).required reads files in the module cache (which is read-only) or files in replacement directories (not expected to be written, perhaps naïvely).

So I guess the surprising thing to me is that ioutil.ReadFile fails when a file is locked by lockedfile.

From the error message, it looks like go/build.Import invoked go list (happens in module mode). I'm guessing the module where go list was invoked is also a replacement in the module where go/build.Import was called. That seems like an unusual situation, but initTestdataModule in pipeline_test.go looks like it's setting up such a module with a version matching the error message.

I'm not at all familiar with plan9. Is there another way we could implement advisory file locking?

Barring that, we should probably use lockedfile in (*mvsReqs).required.

@jayconrod jayconrod added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 28, 2020
@jayconrod jayconrod added this to the Backlog milestone Apr 28, 2020
@bcmills
Copy link
Member

bcmills commented Apr 28, 2020

The lockedfile package is intended not to care whether the locks are advisory or mandatory. (A correct use of lockedfile shouldn't care which is the case, because it assumes that only instances of the same or similar process are using the file anyway.)

@bcmills
Copy link
Member

bcmills commented Apr 28, 2020

The issue with go/build may have been #34860. Are the subrepo tests failing only for the 1.13 branch, or also for 1.14 and master?

@bcmills
Copy link
Member

bcmills commented Apr 28, 2020

The module configuration in that particular test comes from here:
https://github.com/golang/text/blob/6ca2caf96f159660c33dae334f64e31e5da91752/message/pipeline/pipeline_test.go#L136-L148

To me, that suggests that we should use the lockedfile package to read go.mod files found via file-path replacements, because (unlike the go.mod files in the module cache) those go.mod files may be writable and may be concurrently overwritten by other invocations of the go command.

@millerresearch
Copy link
Contributor Author

Are the subrepo tests failing only for the 1.13 branch, or also for 1.14 and master?

They are failing currently in master.

To me, that suggests that we should use the lockedfile package to read go.mod files found via file-path replacements, because (unlike the go.mod files in the module cache) those go.mod files may be writable and may be concurrently overwritten by other invocations of the go command.

I agree. I'll submit a CL today.

@jayconrod
Copy link
Contributor

Fixed by CL 230838

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge 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