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/lockedfile: clean *.lock files #30409

Open
oiooj opened this issue Feb 26, 2019 · 3 comments
Open

cmd/go/internal/lockedfile: clean *.lock files #30409

oiooj opened this issue Feb 26, 2019 · 3 comments
Labels
modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@oiooj
Copy link
Member

oiooj commented Feb 26, 2019

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

go version go1.12

It is necessary to clean these *.lock files in $GOPATH/pkg/mod/cache/download/server.com/xxx/repo?

image

And these files's size always 0. Maybe we can remove these files after unlock.

From code comment:

// We use a separate lockfile here instead of locking listFile itself because
// we want to use Rename to write the file atomically. The list may be read by
// a GOPROXY HTTP server, and if we crash midway through a rewrite (or if the
// HTTP server ignores our locking and serves the file midway through a
// rewrite) it's better to serve a stale list than a truncated one.

cc @bcmills

@oiooj oiooj added the modules label Feb 26, 2019
@bcmills
Copy link
Contributor

bcmills commented Feb 27, 2019

Maybe we can remove these files after unlock.

The version-specific locks are used in two places:

unlock, err := lockVersion(mod)

unlock, err := lockVersion(mod)

Since those guard the creation of .zip files and extracted directories, it might be possible to remove those lockfiles after the files and directories are created.

@bcmills
Copy link
Contributor

bcmills commented Feb 27, 2019

Deleting lockfiles is subtle, and it would be very easy to introduce races that way. On POSIX systems we use advisory locks, which do not prevent the file from being changed or deleted by another process while locked. Consider the following sequence:

  • Processes A locks list.lock and begins updating the list file.
  • Process B attempts to lock list.lock, but blocks because it is held by A.
  • Process A finishes with the list file and unlocks list.lock.
  • Process B acquires list.lock.
  • Process A deletes list.lock.
  • Process C sees that there is no list.lock file, creates one, and locks it.

Now processes B and C are both updating the list file.

To fix that situation, we would need to always delete the file before unlocking it, and always check the identity of a file after locking it.

  • Processes A locks list.lock and begins updating the list file.
  • Process B attempts to lock list.lock, but blocks because it is held by A.
  • Process A finishes with the list file and unlocks list.lock.
  • Process A deletes list.lock (or renames it away).
  • Process A unlocks the (deleted) list.lock.
  • Process B acquires the (deleted) list.lock.
  • Process C sees that there is no list.lock file, creates one, and locks it.
  • Process B checks the file it locked to ensure that that file is still list.lock, discovers that it is not, and restarts the locking procedure.
  • Process B attempts to lock the new list.lock, but blocks because it is held by C.

That requires a much more intricate locking sequence (B needs to verify the identity of the file it locked), and I'm not entirely sure that it's portable (I don't know whether Windows allows locked files to be removed or renamed).

@bcmills
Copy link
Contributor

bcmills commented Feb 27, 2019

See previously #29434.

@bcmills bcmills changed the title internal/lockedfile: clean *.lock files cmd/go/internal/lockedfile: clean *.lock files Feb 27, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 28, 2019
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modules 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

3 participants