Skip to content
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

Handle completed tasks in ApmAsyncFactory. #282

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fiseni
Copy link

@fiseni fiseni commented Oct 20, 2023

This fixes #281.

I couldn't really find documentation on how to handle sync operations in APM. I tested various options, and it seems the IAsyncResult response should have the CompletedSynchronously set to true to indicate that this is a sync flow. In that case, we're not obliged to call the callback, and that's exactly what we need. But, the Task has a hard-coded false value for this property, so returning Task will never work in this scenario. I just ended up using a new internal type CompletedAsyncResult for this scenario.

This fixes all issues, at least the usages I can think of.

@StephenCleary
Copy link
Owner

CompletedSynchronously was my first thought as well. However, that cannot be the whole story because the alternative solution also uses a Task implementation of IAsyncResult, which has CompletedSynchronously set to false.

@fiseni
Copy link
Author

fiseni commented Oct 21, 2023

For ToBegin

  • If the task is already completed, we're returning the newly defined IAsyncResult implementation CompletedAsyncResult, which has set the CompletedSynchronously to true.
  • If the task is not completed, the flow remains as it is. The ToBegin returns (since we have async void) and the callback will be invoked once the original task is completed.

For ToEnd

  • If the IAsyncResult is CompletedAsyncResult then we know it was a sync flow, and we're returning the Result.
  • If the IAsyncResult is Task then we unwrap and return the result.

What am I missing here? 🤔

@fiseni
Copy link
Author

fiseni commented Oct 22, 2023

I might have misunderstood your comment. What do you mean by "alternative solution"? Are you referring to the solution in this article? It's using a continuation, which is dispatched in a separate thread. That's why it always works and we don't need the sync flow (even if the task returns synchronously).

public static IAsyncResult AsApm<T>(this Task<T> task,
                                    AsyncCallback callback,
                                    object state)
{
    if (task == null)
        throw new ArgumentNullException("task");

    var tcs = new TaskCompletionSource<T>(state);
    task.ContinueWith(t =>
                      {
                         if (t.IsFaulted)
                            tcs.TrySetException(t.Exception.InnerExceptions);
                         else if (t.IsCanceled)
                            tcs.TrySetCanceled();
                         else
                            tcs.TrySetResult(t.Result);

                         if (callback != null)
                            callback(tcs.Task);
                      }, TaskScheduler.Default);
    return tcs.Task;
}

@Pretasoc
Copy link

I saw you already found a solution. So please do not feel pressured in any way by this comment. It just happened, that i stumbled apon an other solution for this problem earlier this day. You may wanna have a look at this Task to apm wrapper.

The difference is not that large. It also uses an async result wrapper. It might be a little more lightweight, thatn allocation a new task completion source, but i cannot judge if thats a noticable difference (i highly doubt it).

@fiseni
Copy link
Author

fiseni commented Jul 8, 2024

Thanks. I'm just a contributor and created the PR only as a suggestion.
@StephenCleary will review it at some convenient time. Perhaps he has other ideas for resolving the issue at hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ApmAsyncFactory interop hangs if a path in async method returns synchronously.
4 participants