Avoid repeating code patterns in the Disposables-namespace. #548
Conversation
| } | ||
|
|
||
| // Atomically swap in the new value and get back the old. | ||
| var b = Interlocked.CompareExchange(ref fieldRef, value, old); |
There was a problem hiding this comment.
The problem with this sentinel technique is that if there are a lots of concurrent SetMultiple, threads may keep retrying on ever so state values. Having old read fresh inside the loop can change the window to a favorable thread-thread interaction.
http://cs.oswego.edu/pipermail/concurrency-interest/2016-September/015339.html
There was a problem hiding this comment.
It's currently just the existing stuff copy and pasted. I would not like to change the actual implementation in this PR.
| /// Gets or sets the underlying disposable. After disposal, the result of getting this property is undefined. | ||
| /// </summary> | ||
| /// <exception cref="InvalidOperationException">Thrown if the resource has already been assigned to.</exception> | ||
| internal static void SetSingle(ref IDisposable fieldRef, IDisposable value) |
There was a problem hiding this comment.
I found it sometimes valuable to have this method return true on success and false on an already disposed state.
| value?.Dispose(); | ||
| } | ||
|
|
||
| internal static void SetMultiple(ref IDisposable fieldRef, IDisposable value) |
There was a problem hiding this comment.
Return bool true for success, false for disposed state?
| } | ||
| } | ||
|
|
||
| internal static void SetSerial(ref IDisposable fieldRef, IDisposable value) |
There was a problem hiding this comment.
Return bool true for success, false for disposed state?
|
|
||
| internal static void Dispose(ref IDisposable fieldRef) | ||
| { | ||
| Interlocked.Exchange(ref fieldRef, BooleanDisposable.True)?.Dispose(); |
There was a problem hiding this comment.
I'd advise bias this against multiple dispose calls by doing a volatile read on fieldRef and checking if not already disposed. Dispose calls fly around pretty frequently in Rx.NET and I'm not sure generally those are at most 1-2 per target.
Also returning bool true if the current thread managed to dispose the contents could be valuable.
There was a problem hiding this comment.
I don't know what is more performant. A volatile read isn't free either.
| /// <summary> | ||
| /// Gets or sets the underlying disposable. After disposal, the result of getting this property is undefined. | ||
| /// </summary> | ||
| internal static IDisposable Get(ref IDisposable fieldRef, bool returnDefaultWhenDisposed) |
There was a problem hiding this comment.
I'd split this into two, one guarding against the leak, the others not. Saving a branch on cases where there is no worry about leaking the reference would be beneficial.
|
@akarnokd Good to go like this? |
| /// </summary> | ||
| internal static IDisposable GetValue(ref IDisposable fieldRef) | ||
| { | ||
| var current = Volatile.Read(ref fieldRef); |
There was a problem hiding this comment.
Returning null when the contents are disposed is misleading. Maybe there is no need for this method after all since I can just do a Volatile.Read(ref field) when I need the exact result.
There was a problem hiding this comment.
I want the not-leaking-the-sentinel part there
No description provided.