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

Lazy initialize UUID for Background context #786

Closed
wants to merge 1 commit into from

Conversation

ibuildthecloud
Copy link
Contributor

Fixes #782

This will lazy initialize the UUID using sync/once. Given that in the happy path of already initialized value, sync/once does a atomic.LoadUint32() instead of a full Lock(), I can't imagine this will have any real performance impact.

I don't really know under what context this is used so not totally sure if a tests should be added for this or if in general it should be obvious if this code works because of other tests.

Fixes distribution#782

Signed-off-by: Darren Shepherd <darren@rancher.com>
@aaronlehmann
Copy link
Contributor

This looks okay to me. I agree with you that it's better not to do UUID generation in init, and I doubt performance is the primary consideration here if we're using strings as keys for this lookup. But I'm curious to hear what @stevvooe thinks.

// code. For various reasons random could not be available
// https://github.com/docker/distribution/issues/782
ic.id = uuid.Generate().String()
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a chance to return an empty Id, as Once does not prevent other goroutines to return valur

Copy link
Contributor

Choose a reason for hiding this comment

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

That's an interesting point, but I think the code is actually correct. once.Do will block in other threads on the slow path mutex. The mutex unlock involves a memory barrier, so when Do returns in the other goroutines, the write to ic.id will be visible on those threads.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my bad. It prevents. Shame on me :(

@stevvooe
Copy link
Collaborator

@ibuildthecloud This looks fine. Move the once declaration to the struct and we should be good to merge.

@tiborvass
Copy link
Contributor

Removed my approval, I also need this:

diff --git a/context/logger.go b/context/logger.go
index b0f0c50..78e4212 100644
--- a/context/logger.go
+++ b/context/logger.go
@@ -3,8 +3,6 @@ package context
 import (
    "fmt"

-   "github.com/docker/distribution/uuid"
-
    "github.com/Sirupsen/logrus"
 )

@@ -101,8 +99,3 @@ func getLogrusLogger(ctx Context, keys ...interface{}) *logrus.Entry {

    return logger.WithFields(fields)
 }
-
-func init() {
-   // inject a logger into the uuid library.
-   uuid.Loggerf = GetLogger(Background()).Warnf
-}

@tiborvass
Copy link
Contributor

Ping @stevvooe @ibuildthecloud

@stevvooe
Copy link
Collaborator

Closing for #792.

@stevvooe stevvooe closed this Jul 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants