use a common locking mechanism (filestream) for all platforms while writing to settings file #725

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@rohit21agrawal
Contributor

rohit21agrawal commented Jun 30, 2016

This is the correct solution to the issue : NuGet/Home#2860

This makes use of a synchronized version of file locking by acquiring a filestream handle on a lock file. This approach works across all platforms and can do inter-process synchronization too.

CC: @joelverhagen @yishaigalatzer @alpaix @rrelyea

(Once approved, will revert 584723b checked in via pull request : #720

@@ -176,5 +244,8 @@ private static char ToHexChar(int input)
return (char)(input + 0x30);
}
}
+
+ private const int NumberOfRetries = 3000;

This comment has been minimized.

@joelverhagen

joelverhagen Jun 30, 2016

Member

nit: constants at the top

@joelverhagen

joelverhagen Jun 30, 2016

Member

nit: constants at the top

@@ -176,5 +244,8 @@ private static char ToHexChar(int input)
return (char)(input + 0x30);
}
}
+
+ private const int NumberOfRetries = 3000;
+ private const int SleepDuration = 10;

This comment has been minimized.

@joelverhagen

joelverhagen Jun 30, 2016

Member

nit: use TimeSpan to be explicit on units

@joelverhagen

joelverhagen Jun 30, 2016

Member

nit: use TimeSpan to be explicit on units

This comment has been minimized.

@alpaix

alpaix Jun 30, 2016

Contributor
TimeSpan.FromMilliseconds(10);
@alpaix

alpaix Jun 30, 2016

Contributor
TimeSpan.FromMilliseconds(10);
@@ -24,7 +24,7 @@ public static class ConcurrencyUtilities
}
// limit the number of unauthorized, this should be around 30 seconds.
- var unauthorizedAttemptsLeft = 3000;

This comment has been minimized.

@joelverhagen

joelverhagen Jun 30, 2016

Member

There's a test I think to verify the correctness of the async locking mechanism. The same test should be executed on the sync version.

@joelverhagen

joelverhagen Jun 30, 2016

Member

There's a test I think to verify the correctness of the async locking mechanism. The same test should be executed on the sync version.

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 30, 2016

Member

⌚️

Member

joelverhagen commented Jun 30, 2016

⌚️

@alpaix

This comment has been minimized.

Show comment
Hide comment
@alpaix

alpaix Jun 30, 2016

Contributor

Please add a test the locking mechanism works, IE it is able to wait and quit the while loop.
⌚️

Contributor

alpaix commented Jun 30, 2016

Please add a test the locking mechanism works, IE it is able to wait and quit the while loop.
⌚️

@rohit21agrawal

This comment has been minimized.

Show comment
Hide comment
@rohit21agrawal

rohit21agrawal Jul 1, 2016

Contributor

@joelverhagen @alpaix incorporated your feedback and added a test.

Contributor

rohit21agrawal commented Jul 1, 2016

@joelverhagen @alpaix incorporated your feedback and added a test.

+ var acquired = verificationSemaphore.Wait(0);
+ if (!acquired)
+ {
+ throw new InvalidOperationException();

This comment has been minimized.

@alpaix

alpaix Jul 1, 2016

Contributor

replace with Assert.True with explanatory error message

@alpaix

alpaix Jul 1, 2016

Contributor

replace with Assert.True with explanatory error message

+ Thread.Sleep(TimeSpan.FromMilliseconds(1));
+
+ verificationSemaphore.Release();
+ return;

This comment has been minimized.

@alpaix

alpaix Jul 1, 2016

Contributor

redundant?

@alpaix

alpaix Jul 1, 2016

Contributor

redundant?

This comment has been minimized.

@rohit21agrawal

rohit21agrawal Jul 1, 2016

Contributor

not sure what you mean?

@rohit21agrawal

rohit21agrawal Jul 1, 2016

Contributor

not sure what you mean?

This comment has been minimized.

@alpaix

alpaix Jul 1, 2016

Contributor

is return here needed?

@alpaix

alpaix Jul 1, 2016

Contributor

is return here needed?

This comment has been minimized.

@rohit21agrawal

rohit21agrawal Jul 5, 2016

Contributor

no, it's not. fixed.

@rohit21agrawal

rohit21agrawal Jul 5, 2016

Contributor

no, it's not. fixed.

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
Member

joelverhagen commented Jul 5, 2016

:shipit:

@alpaix

This comment has been minimized.

Show comment
Hide comment
@alpaix

alpaix Jul 5, 2016

Contributor

Looks great! :shipit:

Contributor

alpaix commented Jul 5, 2016

Looks great! :shipit:

@rohit21agrawal

This comment has been minimized.

Show comment
Hide comment
@rohit21agrawal

rohit21agrawal Jul 5, 2016

Contributor

Thanks... I have tested this codepath on Mono on Linux, .NET Framework and CoreCLR and haven't noticed any issues..so going ahead and checking this in.

Contributor

rohit21agrawal commented Jul 5, 2016

Thanks... I have tested this codepath on Mono on Linux, .NET Framework and CoreCLR and haven't noticed any issues..so going ahead and checking this in.

@rohit21agrawal

This comment has been minimized.

Show comment
Hide comment
@rohit21agrawal

rohit21agrawal Jul 5, 2016

Contributor

Merged : f607738

Contributor

rohit21agrawal commented Jul 5, 2016

Merged : f607738

@rohit21agrawal rohit21agrawal deleted the dev-ragrawal-filelock branch Jul 11, 2016

pgsin pushed a commit to JurgenCox/compbio-base that referenced this pull request May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment