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

FIx (xml-) config classes for thread-safe issues #1157

Merged
merged 3 commits into from
Jan 24, 2016

Conversation

304NotModified
Copy link
Member

Replacing foreach with for, for thread-safe usage

For forces uses to use lists.

(also for is most of the time more performant)

fixes #1153

@codecov-io
Copy link

Current coverage is 73.78%

Merging #1157 into master will increase coverage by +0.20% as of 1b2622a

@@            master   #1157   diff @@
======================================
  Files          265     265       
  Stmts        15080   15138    +58
  Branches      1656    1660     +4
  Methods          0       0       
======================================
+ Hit          11097   11170    +73
+ Partial        419     418     -1
+ Missed        3564    3550    -14

Review entire Coverage Diff as of 1b2622a


Uncovered Suggestions

  1. +0.08% via ...c/NLog/LogFactory.cs#181...192
  2. +0.08% via ...gingConfiguration.cs#281...291
  3. +0.08% via ...gingConfiguration.cs#244...254
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@304NotModified 304NotModified added bug Bug report / Bug fix waiting for review labels Jan 11, 2016
@304NotModified 304NotModified added this to the 4.3 milestone Jan 11, 2016
@304NotModified 304NotModified changed the title FIx xml) config classes for thread-safe issues FIx (xml-) config classes for thread-safe issues Jan 11, 2016
@s-sreenath
Copy link
Contributor

Wouldn't it be better if you expose a lock variable and use the same during modification of the list?

@304NotModified
Copy link
Member Author

Good question. Well locks are more expensive then creating objects. (CPU, lock is some kind of thread sleep) And when the collection is changed while processing, it's isn't wrong to continue progressing the old collection.

Or do you see it otherwise?

@s-sreenath
Copy link
Contributor

@304NotModified I don't have any other thoughts on the locking. But I do have few questions and comments on the changes.

@@ -302,8 +308,10 @@ public void Uninstall(InstallationContext installationContext)
internal void Close()
{
InternalLogger.Debug("Closing logging configuration...");
foreach (ISupportsInitialize initialize in this.configItems.OfType<ISupportsInitialize>())
var supportsInitializesList = this.configItems.OfType<ISupportsInitialize>().ToList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create method similar to GetInstallableItems

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I needed a parameters for the reverse() in this case (performance)

@304NotModified
Copy link
Member Author

Thanks for the review @page-not-found .

I applied most of your suggestions, see code comment . Please double check. If you think it's OK, just merge it :)

@s-sreenath
Copy link
Contributor

Sure. I just now added some more comments. Can you also check to see there
is no merge conflicts?
On Jan 15, 2016 14:01, "Julian Verdurmen" notifications@github.com wrote:

Thanks for the review @page-not-found https://github.com/Page-Not-Found
.

I applied most of your suggestions, see code comment . Please double
check. If you think it's OK, just merge it :)


Reply to this email directly or view it on GitHub
#1157 (comment).

@s-sreenath
Copy link
Contributor

I don't see the Merge pull request button. it is disabled.

@304NotModified
Copy link
Member Author

(Travis has some issues, e.g. timeouts when restoring packages, in multiple branches seen)

@304NotModified
Copy link
Member Author

@page-not-found if you check this again, please do.

I reverted (and squashed) some previous commutes. I use now every time a variable with .ToList() - which makes a new list always, see reference source: http://referencesource.microsoft.com/#System.Core/System/Linq/Enumerable.cs,e276d6892241255b

@304NotModified
Copy link
Member Author

@page-not-found, you're now a gladiator? ;)

@s-sreenath
Copy link
Contributor

I am good with all of the code now. But I still prefer to use the for construct just because it does not create a enumerator in run time.

One small suggestion, when you have just one line with in a if or for block. Don't need {}

@bhaeussermann
Copy link
Contributor

One small suggestion, when you have just one line with in a if or for block. Don't need {}

That's a matter of style ;). I personally prefer omitting {} when not needed (to keep the code more concise), but others like to include them always. I guess it makes single-line code blocks more visually explicit and consistent with other blocks.

@304NotModified
Copy link
Member Author

if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0)
    goto fail;
if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
    goto fail;
    goto fail;  
if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0)
    goto fail;

err = sslRawVerify(...);

The enormous SSL fail of Apple (2014)

@s-sreenath
Copy link
Contributor

I agree will you both. We are good to merge.

@304NotModified
Copy link
Member Author

Thanks for checking @page-not-found!

304NotModified added a commit that referenced this pull request Jan 24, 2016
FIx (xml-) config classes for thread-safe issues
@304NotModified 304NotModified merged commit df8e830 into master Jan 24, 2016
@304NotModified 304NotModified deleted the fix-ValidateConfig-thread-safe branch February 3, 2016 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug report / Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants