Replace ActionItem replace with a more strongly typed approach to safe a few allocations #188
Conversation
|
@xinchen10 Thanks for looking into this |
| _ = Task.Factory.FromAsync( | ||
| (stateParameter, c, callbackState) => ((Action<TState>)callbackState).BeginInvoke(stateParameter, c, callbackState), | ||
| r => ((Action<TState>)r.AsyncState).EndInvoke(r), state, | ||
| callback); | ||
| } |
There was a problem hiding this comment.
If there is an exception from callback, the process will crash with the current code due to unhandled exception. After the change it will not happen because the exception is preserved in the task but no one is awaiting on it.
There was a problem hiding this comment.
I wasn't aware that you want the application to crash in such a case which I find super awkward since you are essentially a library and the behavior should be up to the application to decide
In theory I could try catch on end invoke and queue the exception as a waitcallback. Thoughts?
ThreadPool.QueueUserWorkItem(new WaitCallback(ignored =>
{
throw originalException;
}));
There was a problem hiding this comment.
As a library it has to consider exceptions from callbacks fatal because there is no safe strategy to handle it. One example is Timer from the framework. I believe all callbacks from the library handle exceptions as appropriate. This method is also used to invoke user callbacks. In case of user callbacks not handling exceptions it should let the app crash.
There was a problem hiding this comment.
I've added it but I'm no longer sure it is worth it. Otherwise I drop it. No hard feelings
…e a few allocations
danielmarbach
force-pushed
the
action-schedule
branch
from
b292456
to
788aeff
Compare
|
Close the PR for now. Will revisit later if needed. |
No description provided.