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

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

Comments

@bcmills
Copy link
Member

@bcmills 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

@jclulow
Copy link
Contributor

@jclulow 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
Member Author

@bcmills bcmills commented Nov 15, 2019

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

@bradfitz bradfitz commented Nov 15, 2019

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

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 5, 2020

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
…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

@gopherbot gopherbot commented Sep 16, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 16, 2020

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

@gopherbot gopherbot closed this in 7f24142 Sep 17, 2020
@bcmills bcmills modified the milestones: Backlog, Go1.16 Sep 17, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 28, 2020

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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.