Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Fix for file locking on filesystems that don't support hard linking #1117

Closed
wants to merge 1 commit into from

Conversation

matjam
Copy link
Contributor

@matjam matjam commented Sep 3, 2017

What does this do / why do we need it?

We originally chose the locking library github.com/nightlyone/lockfile for our file locking needs. Unfortunately, it relies on hard linking to work.

This PR switches us to github.com/theckman/go-flock which uses the native flock() syscall on posix systems and LockFileEx() on Windows, which should work correctly on all filesystems that support file locking.

What should your reviewer look out for in this PR?

Any incorrect behavior.

I have tried to keep the original behavior of retrying every second and printing a message the first time it fails to lock, and a message every 15 seconds after that.

Do you need help or clarification on anything?

This is a very existential question.

Which issue(s) does this PR fix?

fixes #947
fixes #913
fixes #1261

@matjam
Copy link
Contributor Author

matjam commented Sep 3, 2017

the CI build seems to indicate that either the tests create a deadlock, or there's a problem with the PR.

@matjam
Copy link
Contributor Author

matjam commented Sep 3, 2017

@sdboyer I'm assumin the CI build uses docker; do you know how I can replicate the CI build in my own environment?

@darkowlzz
Copy link
Collaborator

@matjam hi, I have been using this Dockerfile for a few months.

Get the Dockerfile and build a container image:

docker build -t dep-dev .

Run all the tests from dep project directory:

docker run --rm -v $PWD:/go/src/github.com/golang/dep dep-dev su dep -c "sh ~/runtest.sh"

In case the docs in the repo don't help much 😬

Hope this helps.

@sdboyer
Copy link
Member

sdboyer commented Sep 10, 2017

@matjam we're not actually using docker. maybe that's a build mode we should add to travis, but it's not one we have currently.

does @darkowlzz's suggestion help for local replication?

@sdboyer
Copy link
Member

sdboyer commented Sep 14, 2017

fyi, we're thinking about actually just providing an option, controllable via an env var, that just omits the use of the locking file entirely. it's a caveat emptor-type situation, but it at least gives most users that are struggling some recourse here without us having to tackle every possible fs/os scenario in existence.

@sdboyer
Copy link
Member

sdboyer commented Sep 19, 2017

bump

@sdboyer
Copy link
Member

sdboyer commented Sep 26, 2017

that option to skip it entirely is #1206, btw

@sdboyer
Copy link
Member

sdboyer commented Sep 30, 2017

this'll need to be refactored, now that #1206 is in.

Copy link
Member

@sdboyer sdboyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs refactor

@matjam
Copy link
Contributor Author

matjam commented Oct 1, 2017

Will take a look. Thanks, Sam.

@sdboyer
Copy link
Member

sdboyer commented Oct 1, 2017

awesome, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants