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

Jump to bottom

syscall: add Flock implementation for Illumos #35618

Closed
bcmills opened this issue Nov 15, 2019 · 7 comments
Closed

syscall: add Flock implementation for Illumos #35618

bcmills opened this issue Nov 15, 2019 · 7 comments
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-illumos
Milestone
Go1.16

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 15, 2019

On most Unix and Unix-like platforms, the Go syscall package provides the Flock function (and the associated constants LOCK_EX, LOCK_SH, LOCK_UN).

That functionality is not provided for Solaris, because (as far as I am aware) Oracle Solaris still does not provide the corresponding system call (see also #24684). However, it appears that the Illumos kernel has supported flock since 2015 (https://www.illumos.org/issues/3252).

We should update the syscall package to provide Flock on illumos. Then we can switch cmd/go/internal/lockedfile/internal/filelock over to use it, and hopefully avoid apparent kernel bugs that affect the current Fcntl implementation used on AIX and Solaris (#32817).

CC @jclulow @gdamore @tklauser
@bcmills bcmills added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-illumos labels Nov 15, 2019
@bcmills bcmills added this to the Backlog milestone Nov 15, 2019
@bcmills bcmills mentioned this issue Nov 15, 2019
Closed
@jclulow
Copy link
Contributor

jclulow commented Nov 15, 2019

It was my impression that the syscall package was effectively frozen at this stage. Is that just from an exposed API perspective? That is, is this OK because there's already Flock in there for other platforms?

@bcmills
Copy link
Contributor Author

bcmills commented Nov 15, 2019 •
edited

My personal view is that it's ok because:

  1. there is already a Flock implementation (with the same semantics) on other platforms, and
  2. we would use Flock within the standard library.

(Either of those on its own might not justify extending the syscall package for that configuration, but given that Flock already exists it's simpler not to require extra build tags in order to use it from some other package.)

@bradfitz
Copy link
Contributor

Yeah, it's okay to modify syscall for the reasons Bryan mentioned. (API already existing + for internal use)

@gopherbot
Copy link

Change https://golang.org/cl/222277 mentions this issue: cmd/go/internal/modload: use a global lockfile to avoid spurious EDEADLK on AIX and Solaris

gopherbot pushed a commit that referenced this issue Mar 10, 2020
cmd/go/internal/lockedfile: use a retry loop to suppress EDEADLK on A…
c6f678b 
…IX and Solaris

AIX, Solaris, and Illumos all appear to implement fcntl deadlock
detection at the granularity of processes. However, we are acquiring
and releasing file locks on individual goroutines running
concurrently: our locking occurs at a much finer granularity. As a
result, these platforms occasionally fail with EDEADLK errors, when
they detect locks that would be _misordered_ in a single-threaded
program but are safely _unordered_ in a multi-threaded context.

To work around the spurious errors, we treat EDEADLK as always
spurious, and retry the failing system call with a bounded exponential
backoff. This approach may introduce substantial latency since we no
longer benefit from kernel-scheduled wakeups in case of collisions,
but high-latency operations seem better than spurious failures.

Updates #33974
Updates #35618
Fixes #32817

Change-Id: I58b2c6a0f143bce55d6460fd4ddc3db83577ada7
Reviewed-on: https://go-review.googlesource.com/c/go/+/222277
Reviewed-by: Jay Conrod <jayconrod@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/255377 mentions this issue: unix: add Flock on illumos

@gopherbot
Copy link

Change https://golang.org/cl/255258 mentions this issue: syscall, cmd/go/internal/lockedfile/internal/filelock: add and use Flock on illumos

@bcmills bcmills modified the milestones: Backlog, Go1.16 Sep 17, 2020
@gopherbot
Copy link

Change https://golang.org/cl/257940 mentions this issue: cmd/go/internal/lockedfile/internal/filelock: remove stale TODO comment

gopherbot pushed a commit that referenced this issue Sep 29, 2020
@tklauser
cmd/go/internal/lockedfile/internal/filelock: remove stale TODO comment
15c8925 
This was addressed by CL 255258.

Updates #35618

Change-Id: I8dd5b30a846f2d16a3d4752304861d7d2178d1cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/257940
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-illumos
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
4 participants
@bradfitz @jclulow @bcmills @gopherbot