Fix #16274 - LazyInitializer.EnsureInitialized overload for reference types. by WinCPP · Pull Request #16507 · dotnet/corefx

WinCPP · 2017-02-27T20:16:51Z

This PR is for the implementation of new overloads of LazyInitializer.EnsureInitialized for reference types.

@stephentoub @terrajobst @karelz Kindly review.

Depends on PR #9831 in CoreClr for compilation to succeed.

Thanks,
Mandar

WinCPP · 2017-02-27T20:17:18Z

Also please note that I missed creating a new branch in my fork for 16274 and commits ended up into 3862 branch. So you may see lots of commits below but the files changed tab shows only the relevant files. I tried creating a new branch and cherry-picking changes, but I got exhausted with git. Hope that is fine.

Thanks,
Mandar

WinCPP · 2017-02-27T20:21:10Z

+            Assert.Equal(1, ClassWithInitFunction.CountInitialized);
+        }
+
+        private class ClassBase


I had created this as common base with counters and sleep function to be used for ctor based API overload test case. Now that is removed, but I missed merging this into the derived class...

WinCPP · 2017-02-27T20:36:39Z

@stephentoub @karelz seems like the builds are failing because of new API implementation in coreclr not yet being available on corefx side. Did I file the PR too early? Kindly advise.

stephentoub · 2017-02-27T20:45:13Z

seems like the builds are failing because of new API implementation in coreclr not yet being available on corefx side

Right, this won't work until the CoreCLR changes are merged and resulting binaries are consumed into CoreFx.

Did I file the PR too early? Kindly advise.

It's fine. We can re-run it once the changes are available. It'd be good to link to that dependency PR from this PR description, though.

As an aside, before you submit PRs, please rebase your branch on top of the latest master, or at least squash it; there are 28 commits in this PR, most of which are merge commits.

WinCPP · 2017-02-27T20:54:22Z

As an aside, before you submit PRs, please rebase your branch on top of the latest master, or at least squash it; there are 28 commits in this PR, most of which are merge commits.

Actually, when I click 'Sync' button on github UI, it seems to be doing the push to remote, along with pull, and that is how all my commits were pushed to remote. Is it ok in this repository to use 'git push --force-with-lease' to push the rebased commit...?

stephentoub · 2017-02-27T20:56:09Z

Is it ok in this repository to use 'git push --force-with-lease' to push the rebased commit...?

You don't have push access to the dotnet/corefx repo. But you can push whatever you want to your WinCPP/corefx repo, and any changes you make to your issue-3862 branch will be reflected in this PR.

WinCPP · 2017-02-28T02:42:30Z

@stephentoub @terrajobst @karelz I'm not sure what happened, but the squashing that I did, this PR got closed. I have reopened it. Now I see 1 commit with 2 files. Please check and review.

Thanks!

stephentoub · 2017-03-01T16:09:15Z

+            string f = null;
+            object fLock = null;
+            Assert.Equal(strTemplate, LazyInitializer.EnsureInitialized(ref f, ref fLock, () => strTemplate));
+            Assert.Equal(strTemplate, f);


We should also Assert.NotNull(fLock)

stephentoub · 2017-03-01T16:10:07Z

+            string g = strTemplate;
+            object gLock = null;
+            Assert.Equal(strTemplate, LazyInitializer.EnsureInitialized(ref g, ref gLock, () => strTemplate + "bar"));
+            Assert.Equal(strTemplate, g);


Here we should Assert.Null(gLock)

stephentoub

Thanks

stephentoub · 2017-03-02T13:15:38Z

@WinCPP, this is failing because the added API doesn't yet exist in corert. Two things:

Can you port your addition to https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs ?
For this PR to go through, we'll need to suppress the warning:

D:\j\workspace\AllConfigurat---c1564d53\Tools\ApiCompat.targets(53,5): error : MembersMustExist : Member 'System.Threading.LazyInitializer.EnsureInitialized<T>(T, System.Object, System.Func<T>)' does not exist in the implementation but it does exist in the contract. [D:\j\workspace\AllConfigurat---c1564d53\src\System.Threading\src\System.Threading.csproj]

That can be done by editing the https://github.com/dotnet/corefx/blob/master/src/System.Threading/src/ApiCompatBaseline.uapaot.txt file; you can use this recent PR as an example:
https://github.com/dotnet/corefx/pull/16561/files

stephentoub · 2017-03-02T13:39:56Z

@WinCPP, why did you close this?

WinCPP · 2017-03-02T13:41:32Z

By mistake... I was trying to squash commit and same that as other day happened... Can you please help reopen it... I don't know what happened...

I have got the test case changes ready and I was trying to commit it... with force-with-lease... the 'git status' showed files in green color so I did a git push... (this was after git reset --soft git merge-base origin/master origin/issue-3862... looks like even if the files are in green, it requires a commit before git push --force-with-lease...

stephentoub · 2017-03-02T13:43:50Z

I'm unable to because the new commits you added are no longer in your branch, so there's nothing new for the PR to display.
https://github.com/WinCPP/corefx/commits/issue-3862

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.

WinCPP · 2017-03-02T13:48:16Z

Phew... back open... I will do the changes that you mentioned above now... @stephentoub does it look ok from your side?

WinCPP · 2017-03-02T13:54:58Z

            Assert.NotNull(LazyInitializer.EnsureInitialized(ref a, ref aInit, ref aLock));
            Assert.NotNull(a);
+            Assert.True(aInit);
+            Assert.NotNull(aLock);


@stephentoub I have added asserts for true / not null in earlier test cases as well. Please let me know if this is ok...

…3862

karelz · 2017-03-02T16:14:16Z

@stephentoub for my own education - how is CoreRT API surface/implementation propagated to CoreFX? Is there similar regular ingestion like for CoreCLR?

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.

WinCPP · 2017-03-02T16:27:58Z

@stephentoub I have done the two tasks

Porting code to corert: pull request link.
Done the modifications to suppress warnings.

Please check.

stephentoub · 2017-03-02T16:39:30Z

Is there similar regular ingestion like for CoreCLR?

Yes

stephentoub · 2017-03-02T18:41:10Z

 CannotRemoveBaseTypeOrInterface : Type 'System.Threading.Tasks.Task' does not implement interface 'System.IDisposable' in the implementation but it does in the contract.
 MembersMustExist : Member 'System.Threading.Tasks.Task.Dispose()' does not exist in the implementation but it does exist in the contract.
 MembersMustExist : Member 'System.Threading.Tasks.Task.Dispose(System.Boolean)' does not exist in the implementation but it does exist in the contract.
+MembersMustExist : Member 'System.Threading.LazyInitializer.EnsureInitialized<T>(T, System.Object, System.Func<T>)'


This needs to be the full error message, as in the other cases in the file.

I'm sorry. While cutting out the trailing csproj in compilation error, I chewed up a bit more. I should have seen other lines. Honestly, been doing silly mistakes over my limit. Need to introspect why.

WinCPP · 2017-03-02T20:39:17Z

@stephentoub Is the above edit sufficient... because those two builds are still failing with same error... I hope I'm not missing something else.

stephentoub · 2017-03-02T20:40:59Z

Is the above edit sufficient

It's the wrong file. It needs to be the one in src\System.Threading\ rather than src\System.Runtime

WinCPP · 2017-03-02T21:02:23Z

It's the wrong file. It needs to be the one in src\System.Threading\ rather than src\System.Runtime
😶 hope it goes through...

stephentoub

Thanks!

…for reference types. (dotnet/corefx#16507) * 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. Commit migrated from dotnet/corefx@bb8a57c

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

WinCPP commented Feb 27, 2017

View reviewed changes

WinCPP closed this Feb 28, 2017

WinCPP force-pushed the issue-3862 branch from 185e03c to 4c12cfa Compare February 28, 2017 02:28

WinCPP reopened this Feb 28, 2017

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

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

karelz assigned WinCPP, stephentoub, alexperovich and kouvel Feb 28, 2017

WinCPP force-pushed the issue-3862 branch 3 times, most recently from 719ee9f to 3188e6d Compare March 1, 2017 03:31

stephentoub reviewed Mar 1, 2017

View reviewed changes

stephentoub approved these changes Mar 1, 2017

View reviewed changes

WinCPP closed this Mar 2, 2017

WinCPP force-pushed the issue-3862 branch from 82959c8 to e7c0d73 Compare March 2, 2017 13:38

LazyInitializer.EnsureInitialized overload for reference types that does

53cb5a8

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.

WinCPP reopened this Mar 2, 2017

dnfclas added the cla-already-signed label Mar 2, 2017

WinCPP commented Mar 2, 2017

View reviewed changes

Merge remote-tracking branch 'refs/remotes/dotnet/master' into issue-…

b63255c

…3862

LazyInitializer.EnsureInitialized overload for reference types that does

93de8cb

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.

Merge branch 'master' into issue-3862

0ca3ee2

stephentoub reviewed Mar 2, 2017

View reviewed changes

Update ApiCompatBaseline.uapaot.txt

2daa31e

WinCPP added 2 commits March 3, 2017 02:16

Update ApiCompatBaseline.uapaot.txt

393548f

Update ApiCompatBaseline.uapaot.txt

36a8ceb

stephentoub approved these changes Mar 3, 2017

View reviewed changes

stephentoub merged commit bb8a57c into dotnet:master Mar 3, 2017

karelz modified the milestone: 2.0.0 Mar 7, 2017

WinCPP mentioned this pull request Apr 8, 2017

Issue #15622 New overload Dictionary.Remove #18109

Merged

Conversation

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

Uh oh!

Uh oh!

WinCPP commented Feb 27, 2017

Uh oh!

WinCPP Feb 27, 2017

Choose a reason for hiding this comment

Uh oh!

WinCPP commented Feb 27, 2017

Uh oh!

stephentoub commented Feb 27, 2017

Uh oh!

WinCPP commented Feb 27, 2017

Uh oh!

stephentoub commented Feb 27, 2017

Uh oh!

WinCPP commented Feb 28, 2017

Uh oh!

stephentoub Mar 1, 2017

Choose a reason for hiding this comment

Uh oh!

stephentoub Mar 1, 2017

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 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented Mar 2, 2017

Uh oh!

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

Uh oh!

Uh oh!

stephentoub commented Mar 2, 2017

Uh oh!

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

Uh oh!

Uh oh!

WinCPP Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

karelz commented Mar 2, 2017

Uh oh!

WinCPP commented Mar 2, 2017

Uh oh!

stephentoub commented Mar 2, 2017

Uh oh!

stephentoub Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

WinCPP Mar 2, 2017

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

stephentoub commented Mar 2, 2017

Uh oh!

WinCPP commented Mar 2, 2017

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

WinCPP commented Feb 27, 2017 •

edited

Loading

stephentoub commented Mar 2, 2017 •

edited

Loading

WinCPP commented Mar 2, 2017 •

edited

Loading

WinCPP commented Mar 2, 2017 •

edited

Loading

WinCPP commented Mar 2, 2017 •

edited

Loading