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

incorrect interface #33

Closed
KaiserKarel opened this issue Oct 21, 2018 · 4 comments · Fixed by moby/moby#40045, docker/cli#2152 or #34
Closed

incorrect interface #33

KaiserKarel opened this issue Oct 21, 2018 · 4 comments · Fixed by moby/moby#40045, docker/cli#2152 or #34

Comments

@KaiserKarel
Copy link

Flock does not exactly follow the sync.Locker interface, as it's Lock() method returns an error. sync.Mutex panics in certain occasions. I think we could have a version of flock which also panics instead of returning errors, to ensure we have the correct interface.

@theckman
Copy link
Member

I think panics being an intentional design decision in an interface is not a good choice, and isn't something I'd personally want to support. If you'd like to work with that interface instead, I think it's better to write a shim that wraps the flock library, versus complicating this package further.

@acln0
Copy link
Member

acln0 commented Oct 22, 2018

Flock does not exactly follow the sync.Locker interface

Indeed, it does not. I don't think it should, either, because the semantics don't match. sync.Locker is not the right interface if lock and unlock operations may fail, which is the case for flock operations. To wit, perhaps the package-level documentation should be adjusted such that it does not mention sync.Locker anymore. I'll send a PR.

acln0 added a commit that referenced this issue Oct 22, 2018
(*Flock).Lock and (*Flock).Unlock return errors, unlike the Lock and
Unlock methods specified by sync.Locker. Therefore, don't mention
sync.Locker in package-level documentation, to avoid confusion.

Fixes #33.
@KaiserKarel
Copy link
Author

Either way seems fine to me. There are way more things that may panic with filelocks, so I get the aversion to panics. Simply fixing the Readme removes confusion as well.

Will close since I don't think much can be said, more of a sematics problem.

@KaiserKarel
Copy link
Author

Either way seems fine to me. There are way more things that may panic with filelocks, so I get the aversion to panics. Simply fixing the Readme removes confusion as well.

thaJeztah added a commit to thaJeztah/docker that referenced this issue Oct 6, 2019
full diff: gofrs/flock@v0.7.0...v0.7.1

- gofrs/flock#34 don't mention sync.Locker in package documentation
    - fixes gofrs/flock#33 incorrect interface
- gofrs/flock#35 Fix linting issues and add goreportcard badge

Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this issue Oct 7, 2019
full diff: gofrs/flock@v0.7.0...v0.7.1

- gofrs/flock#34 don't mention sync.Locker in package documentation
    - fixes gofrs/flock#33 incorrect interface
- gofrs/flock#35 Fix linting issues and add goreportcard badge

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: 61a2b7ac94bdd7be69e71af08e26bc2a8bd7e675
Component: engine
burnMyDread pushed a commit to burnMyDread/moby that referenced this issue Oct 21, 2019
full diff: gofrs/flock@v0.7.0...v0.7.1

- gofrs/flock#34 don't mention sync.Locker in package documentation
    - fixes gofrs/flock#33 incorrect interface
- gofrs/flock#35 Fix linting issues and add goreportcard badge

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: zach <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this issue Oct 23, 2019
full diff: gofrs/flock@v0.7.0...v0.7.1

- gofrs/flock#34 don't mention sync.Locker in package documentation
    - fixes gofrs/flock#33 incorrect interface
- gofrs/flock#35 Fix linting issues and add goreportcard badge

Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah added a commit to thaJeztah/cli that referenced this issue Oct 23, 2019
full diff: gofrs/flock@v0.7.0...v0.7.1

- gofrs/flock#34 don't mention sync.Locker in package documentation
    - fixes gofrs/flock#33 incorrect interface
- gofrs/flock#35 Fix linting issues and add goreportcard badge

Signed-off-by: Sebastiaan van Stijn <[email protected]>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this issue Dec 4, 2019
full diff: gofrs/flock@v0.7.0...v0.7.1

- gofrs/flock#34 don't mention sync.Locker in package documentation
    - fixes gofrs/flock#33 incorrect interface
- gofrs/flock#35 Fix linting issues and add goreportcard badge

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Upstream-commit: ad4ca6f0d03115098a61e3fce86173f5bdf2ac6e
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment