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.

Sign up for GitHub

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

Jump to bottom

Proposal: Make AsyncTaskCodeActivity derive from AsyncCodeActivity instead of TaskCodeActivity<object> #186

Open
fubar-coder opened this issue Feb 6, 2022 · 2 comments · May be fixed by #289
Open

Proposal: Make AsyncTaskCodeActivity derive from AsyncCodeActivity instead of TaskCodeActivity<object> #186

fubar-coder opened this issue Feb 6, 2022 · 2 comments · May be fixed by #289

Comments

@fubar-coder
Copy link

The AsyncTaskCodeActivity should not have a Result property. This causes problems in some scenarios with reflection and validation.

There are two ways to solve thje problem:

Create a TaskCodeActivity as a companion for TaskCodeActivity`1 or let the AsyncTaskCodeActivity derive directly from AsyncCodeActivity.

Here's an example implementation, which uses AsyncCodeActivity directly, as the TaskCodeActivity`1 implementation looks unnecessary even though there is a little bit of code duplication:

/// <summary>
/// Activity that executes a <see cref="Task"/> which allows the usage of async/await.
/// </summary>
public abstract class AsyncTaskCodeActivity : AsyncCodeActivity
{
    /// <inheritdoc />
    protected sealed override IAsyncResult BeginExecute(
        AsyncCodeActivityContext context,
        AsyncCallback callback,
        object state)
    {
        var cancellationTokenSource = new CancellationTokenSource();
        context.UserState = cancellationTokenSource;
        return ApmAsyncFactory.ToBegin(
            ExecuteAsync(context, cancellationTokenSource.Token),
            callback,
            state);
    }

    /// <inheritdoc />
    protected sealed override void EndExecute(
        AsyncCodeActivityContext context,
        IAsyncResult result)
    {
        ((CancellationTokenSource)context.UserState).Dispose();
    }

    /// <inheritdoc />
    protected sealed override void Cancel(AsyncCodeActivityContext context) =>
        ((CancellationTokenSource)context.UserState).Cancel();

    /// <summary>
    /// Called to execute this activity.
    /// </summary>
    /// <param name="context">The context.</param>
    /// <param name="cancellationToken">The <see cref="CancellationToken"/>.</param>
    /// <returns>The task.</returns>
    /// <remarks>
    /// The <see cref="AsyncCodeActivityContext.UserState"/> must not be used/altered.
    /// </remarks>
    protected abstract Task ExecuteAsync(
        AsyncCodeActivityContext context,
        CancellationToken cancellationToken);
}
@dmetzgar
Copy link
Contributor

I'm also a bit confused on why there is an AsyncTaskCodeActivity<TResult> and a TaskCodeActivity<TResult>. The latter implies that I could have a synchronous activity that executes a Task or perhaps an activity that starts a Task without waiting for completion. But since everything inherits from AsyncCodeActivity<TResult> I don't see a difference between the two classes such that I could explain when to use one or the other. The new on the Result property that changes it from an OutArgument to an object that is always null would be a bit strange for reflection. I'm curious how that affects CacheMetadata. A PR would be appreciated here. Thanks!

@Foxtrek64
Copy link
Contributor

Foxtrek64 commented Mar 21, 2023 •
edited
Loading

Hi all, perhaps I can provide a bit of history here. I'm the author of the AsyncTaskCodeActivity<TResult> type.

AsyncCodeActivity and AsyncCodeActivity<TResult> are types ported from the System.Activities namespace, effectively CoreWF but when it was maintained by Microsoft for NetFx. These types use the old Begin/EndExecute() methods.

My original proposal had both the generic and non-generic AsyncTaskCodeActivities implementing their respective AsyncCodeActivity and translating the old asynchronous methods into TPL-style.

@lbargaoanu had proposed that, in order to help facilitate code deduplication, the TaskCodeActivity type would be implemented. They also added a reference to Nito.AsyncEx to handle the TPL implementation.

If this TaskCodeActivity base class is causing issues for consumers, I'm perfectly fine deleting it. This should in theory be a non-breaking change since the AsyncCodeActivity type is an internal implementation, but we will need to be careful to ensure the transition does not affect public-facing code adversely.

@Foxtrek64 Foxtrek64 linked a pull request Nov 16, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

AsyncTaskCodeActivity implements AsyncCodeActivity directly Foxtrek64/CoreWF
3 participants
@dmetzgar @fubar-coder @Foxtrek64