Lazy initialize UUID for Background context#786
Lazy initialize UUID for Background context#786ibuildthecloud wants to merge 1 commit intodistribution:masterfrom
Conversation
Fixes distribution#782 Signed-off-by: Darren Shepherd <darren@rancher.com>
|
This looks okay to me. I agree with you that it's better not to do UUID generation in |
There was a problem hiding this comment.
I think there's a chance to return an empty Id, as Once does not prevent other goroutines to return valur
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, my bad. It prevents. Shame on me :(
|
@ibuildthecloud This looks fine. Move the |
|
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
-} |
|
Ping @stevvooe @ibuildthecloud |
|
Closing for #792. |
Fixes #782
This will lazy initialize the UUID using
sync/once. Given that in the happy path of already initialized value,sync/oncedoes aatomic.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.