-
-
Notifications
You must be signed in to change notification settings - Fork 225
Implement process and instance isolation #4498
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4498 +/- ##
==========================================
+ Coverage 73.42% 73.44% +0.01%
==========================================
Files 479 481 +2
Lines 17506 17617 +111
Branches 3480 3507 +27
==========================================
+ Hits 12854 12938 +84
- Misses 3773 3793 +20
- Partials 879 886 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@sentry review |
private bool _acquired; | ||
private bool _disposed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The double-checked locking pattern is correctly implemented, but the _acquired field should be marked as volatile to ensure proper memory visibility across threads. This prevents potential race conditions where one thread might not see the updated value immediately.
private bool _acquired; | |
private bool _disposed; | |
private volatile bool _acquired; |
Did we get this right? 👍 / 👎 to inform future reviews.
{ | ||
throw new InvalidOperationException("Cache directory already locked."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache coordinator acquisition failure throws an InvalidOperationException, but this could make it impossible to create multiple SDK instances in some scenarios. Consider making this non-fatal and falling back to a non-cached transport or using a different isolation strategy.
Did we get this right? 👍 / 👎 to inform future reviews.
/// <remarks> | ||
/// This method can throw all of the same exceptions that the <see cref="FileStream"/> constructor can throw. The | ||
/// caller is responsible for handling those exceptions. | ||
/// </remarks> | ||
public override bool TryCreateLockFile(string path, out Stream fileStream) | ||
{ | ||
// Note that FileShare.None is implemented via advisory locks only on macOS/Linux... so it will stop | ||
// other .NET processes from accessing the file but not other non-.NET processes. This should be fine | ||
// in our case - we just want to avoid multiple instances of the SDK concurrently accessing the cache | ||
fileStream = new FileStream( | ||
path, | ||
FileMode.OpenOrCreate, | ||
FileAccess.ReadWrite, | ||
FileShare.None); | ||
return true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The TryCreateLockFile method can throw all FileStream constructor exceptions but always returns true. This is inconsistent with the interface design pattern where 'Try' methods typically return false on failure rather than throwing. Consider catching exceptions and returning false, or rename the method to indicate it can throw.
/// <remarks> | |
/// This method can throw all of the same exceptions that the <see cref="FileStream"/> constructor can throw. The | |
/// caller is responsible for handling those exceptions. | |
/// </remarks> | |
public override bool TryCreateLockFile(string path, out Stream fileStream) | |
{ | |
// Note that FileShare.None is implemented via advisory locks only on macOS/Linux... so it will stop | |
// other .NET processes from accessing the file but not other non-.NET processes. This should be fine | |
// in our case - we just want to avoid multiple instances of the SDK concurrently accessing the cache | |
fileStream = new FileStream( | |
path, | |
FileMode.OpenOrCreate, | |
FileAccess.ReadWrite, | |
FileShare.None); | |
return true; | |
} | |
public override bool TryCreateLockFile(string path, out Stream fileStream) | |
{ | |
try | |
{ | |
fileStream = new FileStream( | |
path, | |
FileMode.OpenOrCreate, | |
FileAccess.ReadWrite, | |
FileShare.None); | |
return true; | |
} | |
catch | |
{ | |
fileStream = Stream.Null; | |
return false; | |
} | |
} |
Did we get this right? 👍 / 👎 to inform future reviews.
Resolves #2033, Resolves #1067
Note
I'm not 100% happy with the structure of this code - it impacted quite a few mostly unrelated tests so it's kind of "leaky" at the moment. @Flash0ver if you've got ideas on alternative ways this could be done, I'm all ears.
Interprocess Locking
This turns out to be surprisingly difficult.
Something like a named Mutex could be used for this. However Mutexes are thread affine so they need to be released on the same thread that acquired them. Other than setting aside a dedicated thread for the lifetime of the Hub, there's no easy way to make that work.
A named Semaphore would be another option, but these aren't supported on macOS.
In the end I went with the "lock file" solution that @lucas-zimerman suggested. Keeping a lock file specific to each cache directory and opening this with FileShare.None gives us a kind of file based named mutex that we can use to avoid having multiple instances of the Hub trying to process files from the same cache directory.