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

proposal: make the internal lockedfile package public #33974

Open
azr opened this issue Aug 30, 2019 · 26 comments
Open

proposal: make the internal lockedfile package public #33974

azr opened this issue Aug 30, 2019 · 26 comments
Labels
Projects
Milestone

Comments

@azr
Copy link
Contributor

@azr azr commented Aug 30, 2019

An internal filelock pkg can be found here:
https://pkg.go.dev/cmd/go/internal/lockedfile/internal/filelock

A few projects are implementing file locking [1] but they do not seem to be maintained and I think they are not as nice as the internal filelock pkg I mentioned before. As a result some projects are doing their own version of it [2].

I suggest we make the filelock pkg public as I think this could be beneficial to the mass.

I'd be glad to do it given some pointers, like where to put it ?

Thanks !


[1] File lock projects:

https://github.com/gofrs/flock
https://github.com/MichaelS11/go-file-lock
https://github.com/juju/fslock

[2] Projects that implement their own file locking:

terraform

boltdb

@gopherbot gopherbot added this to the Proposal milestone Aug 30, 2019
@mvdan
Copy link
Member

@mvdan mvdan commented Aug 30, 2019

Why not use the exported version at https://godoc.org/github.com/rogpeppe/go-internal? :)

Loading

@smasher164 smasher164 changed the title Proposal: make the internal filelock pkg public proposal: make the internal filelock package public Aug 30, 2019
@azr
Copy link
Contributor Author

@azr azr commented Sep 3, 2019

Hey @mvdan, 🙂 here are a few reasons why I didn't go with it:

  • the filelock of go-internal is in an internal pkg
  • importing go-internal makes my go-deps bigger than necessary ( edit: this not such a big issue )
  • it feels wrong, if I find a bug, then I have to make a pull request to go and then wait for it to get merged and then wait for go-internals to update. It sounds like it has all the annoying effects of having these pkgs not internal anyway.

Loading

@mvdan
Copy link
Member

@mvdan mvdan commented Sep 3, 2019

the filelock of go-internal is in an internal pkg

I'm not sure why that is, perhaps @rogpeppe or @myitcv can answer. Have you looked at https://godoc.org/github.com/rogpeppe/go-internal/lockedfile? That seems like a public, higher-level version of it.

That aside, sure, there are some upsides to having this in the standard library. But I think the downsides are generally greater:

  • Once a package is public, it can never have backwards incompatible changes
  • The standard library can grow, but never shrink in size
  • Once the number of users greatly outgrows just cmd/go, the maintainers will probably have extra work responding to issues, reviewing patches, and fixing bugs that might not even affect cmd/go

I think a far better proposal would be to include this in one of the official external repos, like x/sync. Once that's worked well for a while, then it can be part of the public standard library. This is the path that context took, for example.

Loading

@azr
Copy link
Contributor Author

@azr azr commented Sep 3, 2019

lockedfile fits right to our usage and lockedfile doesn't have the filelock.ErrNotSupported. But yes lockedfile could work here.

Yes, I agree this should probably be in an x/ pkg 🙂, I only suggested we make the filelock pkg public and agree this would be a bit quick to put in the stdlib.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Sep 3, 2019

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 3, 2019

At this point we've been using lockedfile for a full release cycle and it seems mostly fine, modulo what appears to be an AIX kernel bug (#32817). I'd be fine with making it public.

Then the question is where to put it: x/sys, x/exp, x/sync, or someplace else entirely?

Loading

@azr
Copy link
Contributor Author

@azr azr commented Sep 4, 2019

It's right at the intersection of sync and sys.
I think lockedfile should be in x/sync/lockedfile
I think that filelock is a bit lower level and could fit in x/sys/filelock ( if we ever make it public ). But these two are may be a bit redundant.

edit: regroup them both under x/exp/syssync ?

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 4, 2019

@azr, filelock should remain internal — it's much too subtle to use on its own. (In particular, if you want your program to remain portable, you have to be very careful to stick to a narrow subset of the possible modes of use.)

If folks discover other interesting use-cases for filelock, then we can probably add more ergonomic packages to address those use-cases while still keeping filelock internal.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Sep 25, 2019

Given that the author of filelock says it is not ready to be exported, it seems like this is a likely decline.

Leaving open for a week for final comments.

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 25, 2019

To be clear: I support making the lockedfile package itself public — I just want to ensure that the lockedfile/internal/filelock package remains internal to it.

Loading

@azr azr changed the title proposal: make the internal filelock package public proposal: make the internal lockedfile package public Sep 26, 2019
@azr
Copy link
Contributor Author

@azr azr commented Sep 26, 2019

I just want to ensure that the lockedfile/internal/filelock package remains internal to it.

That's fine by me 🙂.

I think I'll put it in x/sync/lockedfile soonish; (unless you think another place is better ).

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 2, 2019

I misunderstood. Thanks for the clarification, @bcmills. Removing the FinalCommentPeriod label.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 2, 2019

I don't think we are ready to add lockedfile to the standard library. It is a peculiar set of functions that does not really line up with the usual things in either os or io. Maybe it would be OK in x/sync but maybe it would be better to start in x/exp to understand if the API needs any adjustments.

If we are going to add a new public package (even in x/exp), I think the next step would be for someone to write a proper proposal doc laying out the API and explaining the decisions, get comments, and so on.

@azr, do you want to do that?

Loading

@azr
Copy link
Contributor Author

@azr azr commented Oct 3, 2019

@rsc gotcha ! Yes I want to ! Where should such a proposal doc be written ? Is there a template for that ?

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Oct 3, 2019

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 9, 2019

Putting proposal on hold for design doc.

Loading

@azr
Copy link
Contributor Author

@azr azr commented Oct 15, 2019

Hello there; I created the proposal ( first ever ! ) here golang/proposal#21; I hope it's good enough please feel free to give feedback 🙂 🙂 !

Loading

azr added a commit to azr/proposal that referenced this issue Oct 16, 2019
azr added a commit to azr/proposal that referenced this issue Oct 16, 2019
azr added a commit to azr/proposal that referenced this issue Oct 16, 2019
@gopherbot
Copy link

@gopherbot gopherbot commented Oct 18, 2019

Change https://golang.org/cl/201277 mentions this issue: design: add proposal doc for 33974

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 6, 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

Loading

gopherbot pushed a commit to golang/proposal that referenced this issue Mar 10, 2020
Make the `cmd/internal/lockedfile` Package public.

Updates golang/go#33974

Change-Id: I2502fad153254d9ddb2bcc96ed6d8ef163940add
GitHub-Last-Rev: d6be79e
GitHub-Pull-Request: #21
Reviewed-on: https://go-review.googlesource.com/c/proposal/+/201277
Reviewed-by: Bryan C. Mills <bcmills@google.com>
@bcmills
Copy link
Member

@bcmills bcmills commented Mar 10, 2020

Loading

@bcmills
Copy link
Member

@bcmills bcmills commented Mar 10, 2020

We also now have a solution for the AIX and Solaris EDEADLK bug pending (CL 222277). The workaround is simple (a retry loop), but the justification and regression test are at least non-trivial.

Loading

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>
@azr
Copy link
Contributor Author

@azr azr commented Jun 12, 2020

Hello @bcmills, can I already start working on this ? Can I help in any manner ? Thanks !

Loading

@arl
Copy link
Contributor

@arl arl commented Jan 8, 2021

Hi @bcmills
Jumping in to query the state of this proposal. Since the proposal-hold label has been removed, as per https://github.com/golang/proposal#hold it seems that this proposal could be updated to active

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 8, 2021

It's in the "incoming" list. We move proposals from "incoming" to "active" as we have bandwidth to handle them. See https://go.googlesource.com/proposal/+/refs/heads/master/README.md .

Loading

@arl
Copy link
Contributor

@arl arl commented Jan 8, 2021

@ianlancetaylor I had missed that bit of information. Thank you

Loading

@tamilmani1989
Copy link

@tamilmani1989 tamilmani1989 commented Oct 15, 2021

any update on making lockedfile package public?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants