CoreFx #16274 Implementation - New overloads of LazyInitializer.EnsureInitialized for reference types by WinCPP · Pull Request #9831 · dotnet/coreclr

WinCPP · 2017-02-27T18:43:37Z

This PR is for the implementation of new overloads of LazyInitializer.EnsureInitialized for reference types. Please refer to https://github.com/dotnet/corefx/issues/16274 for more information.

@stephentoub @terrajobst @karelz Kindly review the changes.

stephentoub · 2017-02-27T18:45:39Z

+        /// <param name="syncLock">A reference to an object used as the mutually exclusive lock for initializing
+        /// <paramref name="target"/>. If <paramref name="syncLock"/> is null, a new object will be instantiated.</param>
+        /// <returns>The initialized value of type <typeparamref name="T"/>.</returns>
+        public static T EnsureInitialized<T>(ref T target, ref object syncLock) where T : class


I don't believe this API was discussed or approved, was it? Wasn't it just the one that takes a delegate?

It was discussed but not concluded (here)... that is why I left the comment above (here). Request you to consider the same as it makes the API symmetric...

@WinCPP any API change has to be first concluded and approved - it is best done in issues. PRs are bad place to do API review discussions. Please revert it for now and restart the discussion in the issue (or create a new one).

removing...

@karelz our comments crossed... yes I understand it is not a good idea. I am removing the API. Build and test is in progress and will push the same.

@WinCPP perfect, thank you!

stephentoub · 2017-02-27T18:48:09Z

+                if (Volatile.Read(ref target) == null)
+                {
+                    target = valueFactory();
+                    if (target == null)


This should be: Volatile.Write(ref target, valueFactory());

Changing this. It will force the write of target.

WinCPP · 2017-02-27T19:48:24Z

@stephentoub , the changes are in. Sorry.

stephentoub · 2017-02-27T20:39:47Z

+            }
+
+            return EnsureInitializedCore<T>(ref target, ref syncLock, valueFactory);
+        }


Nit: I realize you're just copying the style used above; that's fine. We could subsequently shorten / clean this up as:

public static T EnsureInitialized<T>(ref T target, ref object syncLock, Func<T> valueFactory) where T : class => Volatile.Read(ref target) ?? EnsureInitializedCore<T>(ref target, ref syncLock, valueFactory);

stephentoub · 2017-02-28T14:31:05Z

+                    slock = newLock;
+                }
+            }
+


Since this code is now duplicated in two different methods, could you separate it out into a helper? e.g.

private static object EnsureLockInitialized(ref object syncLock) => syncLock ?? Interlocked.CompareExchange(ref syncLock, new object(), null) ?? syncLock;

which you could then use here and in the other function as:

lock (EnsureLockInitialized(ref syncLock)) { ... }

?

Yup. Makes sense. Actually out of habit, I was into the groove of 'don't touch something which already works; follow the style'...

private static object EnsureLockInitialized(ref object syncLock) => syncLock ?? Interlocked.CompareExchange(ref syncLock, new object(), null) ?? syncLock;

~~Quick Qestion: The '?? syncLock' towards end of statement made me scratch my head a bit... typo, not intentional... right...?~~ Really bad question... treated it like Interlocked.Increment which returns the incremented value...

WinCPP · 2017-02-28T18:55:22Z

@stephentoub the changes are ready...

stephentoub · 2017-02-28T23:20:56Z

+
+            // Lazily initialize the lock if necessary and,  then double check if initialization is still required.
+            lock (EnsureLockInitialized(ref slock))
            {


This is not correct. It needs to be:

lock (EnsureLockInitialized(ref syncLock))

By initializing the local, racing threads will all end up creating and using their own slock object. You can delete the slock local.

stephentoub · 2017-02-28T23:21:48Z

+
+            // Lazily initialize the lock if necessary and,  then double check if initialization is still required.
+            lock (EnsureLockInitialized(ref slock))
            {


Same problem. This needs to be ref syncLock, not ref slock. You can delete the local slock.

Yes, agreed. This is because of oversight. My test cases ran successfully because I added one for non-null syncLock to the new LazyInitializer.EnsureInitialized API and was happy with its success. I will add another test case for scenario that the user sends in a null syncLock which would have set off the race condition.

stephentoub · 2017-02-28T23:22:02Z

+        /// <param name="syncLock">A reference to a location containing a mutual exclusive lock. If <paramref name="syncLock"/> is null,
+        /// a new object will be instantiated.</param>
+        /// <returns>Initialized lock object.</returns>
+        private static object EnsureLockInitialized(ref object syncLock) => syncLock ?? Interlocked.CompareExchange(ref syncLock, new object(), null) ?? syncLock;


Nit: please split this across multiple lines to make it easier to read.

types LazyInitializer.EnsureInitialized overload for reference types that does not take boolean for tracking initialization status of the reference type object. Null check on the object parameter is sufficient to whether or not it has been initialized.

stephentoub

Thanks for adding this.

stephentoub · 2017-03-01T12:54:59Z

@jkotas, does the mscorlib.cs ref still need to be updated to include new APIs, or is that now defunct?

jkotas · 2017-03-01T15:00:13Z

mscorlib.cs ref is defunct. We can delete it once the CoreCLR tests are fixed to consume live corefx - they are still stuck on corefx before the corefx build changes.

stephentoub · 2017-03-01T15:01:38Z

Thanks.

…Overloads CoreFx dotnet/coreclr#16274 Implementation - New overloads of LazyInitializer.EnsureInitialized for reference types Commit migrated from dotnet/coreclr@38b0df9

dnfclas added the cla-already-signed label Feb 27, 2017

stephentoub reviewed Feb 27, 2017

View reviewed changes

WinCPP mentioned this pull request Feb 27, 2017

Fix #16274 - LazyInitializer.EnsureInitialized overload for reference types. dotnet/corefx#16507

Merged

WinCPP force-pushed the LazyInitializerNewOverloads branch from f82f946 to a8d9a1a Compare February 28, 2017 02:53

karelz added the area-System.Threading label Feb 28, 2017

stephentoub reviewed Feb 28, 2017

View reviewed changes

WinCPP force-pushed the LazyInitializerNewOverloads branch from a8d9a1a to 283f386 Compare February 28, 2017 16:28

stephentoub reviewed Feb 28, 2017

View reviewed changes

WinCPP force-pushed the LazyInitializerNewOverloads branch from 283f386 to f6533e1 Compare March 1, 2017 03:25

stephentoub approved these changes Mar 1, 2017

View reviewed changes

stephentoub merged commit 38b0df9 into dotnet:master Mar 1, 2017

WinCPP deleted the LazyInitializerNewOverloads branch March 2, 2017 16:39

karelz modified the milestone: 2.0.0 Aug 28, 2017

Conversation

WinCPP commented Feb 27, 2017 • edited by karelz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WinCPP Feb 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WinCPP commented Feb 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WinCPP Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WinCPP commented Feb 28, 2017

Uh oh!

stephentoub Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Mar 1, 2017

Uh oh!

jkotas commented Mar 1, 2017

Uh oh!

stephentoub commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

WinCPP commented Feb 27, 2017 •

edited by karelz

Loading

stephentoub Feb 27, 2017 •

edited

Loading

WinCPP Feb 27, 2017 •

edited

Loading

WinCPP Feb 28, 2017 •

edited

Loading

stephentoub Feb 28, 2017 •

edited

Loading