Review #4
Review #4
Conversation
| @@ -64,6 +64,7 @@ public FutureHandlerException (IFuture future, Delegate handler, string message) | |||
| } | |||
|
|
|||
| internal static class FutureHelpers { | |||
| // REVIEW: Pretty sure I would like to rewrite this RunInThreadThunk subsystem but not 100% sure how | |||
There was a problem hiding this comment.
It's old code so it probably does need it
| @@ -75,6 +76,7 @@ internal static class FutureHelpers { | |||
| try { | |||
| thunk.Invoke(); | |||
| } catch (System.Reflection.TargetInvocationException ex) { | |||
| // REVIEW: Which cases is this catch block for? | |||
There was a problem hiding this comment.
Good question, I don't remember. I know Delegate.Invoke always throws TargetInvocationException so that might be why it's doing this unwrap here. It definitely needed a comment.
There was a problem hiding this comment.
Yeah I have tried to convince Delegate.DynamicInvoke to trigger this exception but I mostly succeeded in it raising "other" exception types... (and that it would not make sense to unwrap to the client)
| @@ -190,6 +192,8 @@ public SignalFuture (bool signaled) | |||
|
|
|||
| public static class Future { | |||
| public enum State : int { | |||
| // REVIEW: I guess Unknown and Disposing are removed because | |||
| // you did not want these states to be part of the public API? | |||
There was a problem hiding this comment.
Yeah I don't want the consumer to look at those and try to do something based on seeing them, though it might make sense to just broadcast them as 'Indeterminate' or something and say you should treat those as a 'please retry later'
There was a problem hiding this comment.
I think that, as both State_Indeterminate and State_Disposing are transitory, not exposing them is what makes the most sense for everyone (in particular, State_Indeterminate should really be understood as "This Future instance is locked with regard to operations that could resolve it", but no client that is outside of Future should treat it differently from "not resolved yet", which is Empty).
However, I guess that, given the existence of the various boolean properties (Resolved/Completed/Disposed/CompletedSuccessfully/CompletedWithError), I am not 100% sure that this enum needs to be public?
There was a problem hiding this comment.
(Of course, I am not saying that it should now be removed - that would break existing clients for no good reason)
| // REVIEW: On a theoretical level, I am worried that, if the enumerable is | ||
| // empty and not detected by the condition below, or if all futures are null | ||
| // then Composite never completes. | ||
| // On a practical level, I suspect that it has not happened often. ;) |
There was a problem hiding this comment.
Good point, I should make sure that it works correctly without hitting this. I think it does, but it may not have test coverage.
I have begun reading through the Threading library, and have made at least a first pass on
Future.cs.You will find my comments on the code itself as code comments.
I also want to say, as a general matter, that all this seems quite good to me - it is clear that a lot of thought went into the library design and implementation, and that you are significantly better than I am. (Which is why there are not many comments right now - at a time I had forgotten how
Interlocked.CompareExchangeworked and had a few erroneous comments ready to go)I hope that my remarks can still be useful to you, and I will be continuing the review by adding commits on the branch.