[WIP] Feature to disable file locking when DEPNOLOCK set #1206

ayang64 · 2017-09-22T19:50:20Z

Add DisableLocking bool members to Ctx and gps.SourceManagerConfig structs.
This effectively communicates DEPNOLOCK from the shell, to Ctx, to
SourceManager.

The member is named DisableLocking to make its zero-value useful.
Add Locker interface which implements TryLock(), Unlock(), and GetOwner()
which lockfile.Lockfile alredy adheres to. This interface replaces the new
type for the lf member of the SourceMgr struct.
Add a FalseLocker type which adheres to the Locker interface which does
nothing.
Conditionally set the lf member of SourceMgr to either an instance of
lockfile.Lockfile or FalseLocker depending on the value of
SourceManagerConfig.DisableLocking.

What does this do / why do we need it?

This pull request adds functionality that checks the DEPNOLOCK environment variable to determine if session manager should attempt to lock against multiple instances of dep runing.

This seems to be a problem particularly when building with CI systems in a Vagrant or Docker type container that uses union mount type file systems.

This environment variable allows us to work around this locking issue in these kind of systems where we can be sure there aren't concurrent runs of dep.

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

I'm interested to know if this is a reasonable approach and I'm open to suggestions of alternate ways to achieve this.

Which issue(s) does this PR fix?

fixes #947

googlebot · 2017-09-22T19:50:23Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

ayang64 · 2017-09-22T21:00:55Z

I signed it!

googlebot · 2017-09-22T21:00:57Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

sdboyer · 2017-09-26T01:51:13Z

having a look - but it seems like you may need to regenerate that commit with different git authorial metadata :)

ayang64 · 2017-09-26T01:53:36Z

ya -- it needs lots of work. i was going to spend some time within the next couple of days getting it to pass code climate and working on agreeing to the cla.

sdboyer

somewhat superficial review, problems are mostly nittish.

i haven't really thought deeply about if there are problems that arise from this, but it's hard to imagine any off the top of my head. this seems like basically what we want.

sdboyer · 2017-09-26T01:52:54Z

internal/gps/source_manager.go

+	GetOwner() (*os.Process, error)
+}
+
+type FalseLocker struct{}


also no need to export this, i don't think?

sdboyer · 2017-09-26T01:53:17Z

internal/gps/source_manager.go

@@ -28,6 +28,26 @@ import (
 // Used to compute a friendly filepath from a URL-shaped input.
 var sanitizer = strings.NewReplacer("-", "--", ":", "-", "/", "-", "+", "-")

+type Locker interface {


shouldn't need to be exported. if there is a reason to, though, it'll need a comment.

sdboyer · 2017-09-26T01:53:55Z

internal/gps/source_manager.go

-	Logger   *log.Logger // Optional info/warn logger. Discards if nil.
+	Cachedir       string      // Where to store local instances of upstream sources.
+	Logger         *log.Logger // Optional info/warn logger. Discards if nil.
+	DisableLocking bool


In keeping with the other items, let's have an inline explanatory comment.

Okay -- will try to address these issues tonight. Thank you!

sdboyer · 2017-09-26T01:55:45Z

internal/gps/source_manager.go

-	lf          *lockfile.Lockfile    // handle for the sm lock file on disk
+	cachedir string // path to root of cache dir
+	// lf          *lockfile.Lockfile    // handle for the sm lock file on disk
+	lf          Locker                // handle for the sm lock file on disk


drop the old prop and update the comment

* Add DisableLocking bool members to Ctx and gps.SourceManagerConfig structs. This effectively communicates DEPNOLOCK from the shell, to Ctx, to SourceManager. The member is named DisableLocking to make its zero-value useful. * Add locker interface which implements TryLock(), Unlock(), and GetOwner() which lockfile.Lockfile alredy adheres to. This interface replaces the new type for the lf member of the SourceMgr struct. * Add a FalseLocker type which adheres to the Locker interface which does nothing. * Conditionally set the lf member of SourceMgr to either an instance of lockfile.Lockfile or FalseLocker depending on the value of SourceManagerConfig.DisableLocking. Signed-off-by: Ayan George <[email protected]>

googlebot · 2017-09-27T01:40:00Z

CLAs look good, thanks!

Signed-off-by: Ayan George <[email protected]>

sdboyer · 2017-09-28T03:34:13Z

context.go

+	GOPATHs        []string    // Other Go paths.
+	Out, Err       *log.Logger // Required loggers.
+	Verbose        bool        // Enables more verbose logging.
+	DisableLocking bool        // When set, disables locking.


we need a little more on this comment here. it's in reference to a rather obscure feature that most people don't know about. with just this context, i would probably expect people to assume that it referred to, say, not writing out Gopkg.lock (which is not something we do at all). instead, maybe:

"When set, no lock file will be created to protect against multiple simultaneous dep processes."

Absolutely. Frankly I wrote something similar originally and changed my mind (something about that comment being much longer than the others -- silly I know).

I'll fix immediately.

Signed-off-by: Ayan George <[email protected]>

sdboyer

just a couple typos. also, would you mind squashing this down?

sdboyer · 2017-09-29T20:30:40Z

internal/gps/source_manager.go

@@ -28,6 +28,41 @@ import (
 // Used to compute a friendly filepath from a URL-shaped input.
 var sanitizer = strings.NewReplacer("-", "--", ":", "-", "/", "-", "+", "-")

+// A locker is responsible for preventing multiple instances of dep from interfereing


nit: s/interfereing/interfering/

sdboyer · 2017-09-29T20:31:52Z

internal/gps/source_manager.go

-	Logger   *log.Logger // Optional info/warn logger. Discards if nil.
+	Cachedir       string      // Where to store local instances of upstream sources.
+	Logger         *log.Logger // Optional info/warn logger. Discards if nil.
+	DisableLocking bool        // True if dep SourceManager shoud NOT lock.


"shoud" typo here. also, let's rewrite this a bit, too:

"True if the SourceManager should NOT use a file lock to protect the Cachedir from multiple processes."

Signed-off-by: Ayan George <[email protected]>

* Disable file locking when DEPNOLOCK set * Add DisableLocking bool members to Ctx and gps.SourceManagerConfig structs. This effectively communicates DEPNOLOCK from the shell, to Ctx, to SourceManager. The member is named DisableLocking to make its zero-value useful. * Add locker interface which implements TryLock(), Unlock(), and GetOwner() which lockfile.Lockfile alredy adheres to. This interface replaces the new type for the lf member of the SourceMgr struct. * Add a FalseLocker type which adheres to the Locker interface which does nothing. * Conditionally set the lf member of SourceMgr to either an instance of lockfile.Lockfile or FalseLocker depending on the value of SourceManagerConfig.DisableLocking. Signed-off-by: Ayan George <[email protected]> * Revert stray edit. Signed-off-by: Ayan George <[email protected]> * Improve comment for DisableLocking Signed-off-by: Ayan George <[email protected]> * Fix comment type-os * Fix comment type-os Signed-off-by: Ayan George <[email protected]> * Fix yet more type-os. Signed-off-by: Ayan George <[email protected]>

ayang64 requested a review from sdboyer as a code owner September 22, 2017 19:50

googlebot added the cla: no label Sep 22, 2017

sdboyer suggested changes Sep 26, 2017

View reviewed changes

sdboyer added the area: gps label Sep 26, 2017

sdboyer mentioned this pull request Sep 26, 2017

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

Closed

googlebot added cla: yes and removed cla: no labels Sep 27, 2017

Revert stray edit.

0088119

Signed-off-by: Ayan George <[email protected]>

sdboyer reviewed Sep 28, 2017

View reviewed changes

ayang64 added 4 commits September 28, 2017 09:25

Improve comment for DisableLocking

3faed10

Signed-off-by: Ayan George <[email protected]>

Fix comment type-os

744d239

Fix comment type-os

0dec1c3

Signed-off-by: Ayan George <[email protected]>

Merge branch 'dep-no-lock' of github.com:ayang64/dep into dep-no-lock

f6a117f

sdboyer suggested changes Sep 29, 2017

View reviewed changes

Fix yet more type-os.

0b746f9

Signed-off-by: Ayan George <[email protected]>

sdboyer approved these changes Sep 29, 2017

View reviewed changes

sdboyer merged commit 3fd5bb3 into golang:master Sep 29, 2017

sdboyer mentioned this pull request Oct 12, 2017

pidfile lock waiting broken in docker containers #1261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Feature to disable file locking when DEPNOLOCK set #1206

[WIP] Feature to disable file locking when DEPNOLOCK set #1206

ayang64 commented Sep 22, 2017 •

edited

Loading

googlebot commented Sep 22, 2017

ayang64 commented Sep 22, 2017

googlebot commented Sep 22, 2017

sdboyer commented Sep 26, 2017

ayang64 commented Sep 26, 2017

sdboyer left a comment

sdboyer Sep 26, 2017

sdboyer Sep 26, 2017

sdboyer Sep 26, 2017

ayang64 Sep 26, 2017

sdboyer Sep 26, 2017

googlebot commented Sep 27, 2017

sdboyer Sep 28, 2017

ayang64 Sep 28, 2017

sdboyer left a comment

sdboyer Sep 29, 2017

sdboyer Sep 29, 2017

[WIP] Feature to disable file locking when DEPNOLOCK set #1206

[WIP] Feature to disable file locking when DEPNOLOCK set #1206

Conversation

ayang64 commented Sep 22, 2017 • edited Loading

What does this do / why do we need it?

What should your reviewer look out for in this PR?

Do you need help or clarification on anything?

Which issue(s) does this PR fix?

googlebot commented Sep 22, 2017

ayang64 commented Sep 22, 2017

googlebot commented Sep 22, 2017

sdboyer commented Sep 26, 2017

ayang64 commented Sep 26, 2017

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayang64 commented Sep 22, 2017 •

edited

Loading