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

Alternative CreateTemporalActivityDefinition #207

Merged
merged 6 commits into from
Mar 26, 2024

Conversation

houseofcat
Copy link
Contributor

@houseofcat houseofcat commented Mar 23, 2024

What Changed

Allow the usage of AsyncServiceProvider in the ServiceProviderExtensions for NET6+ during CreateTemporalActivityDefinitions.

  • Backwards compatible.
  • Choose the package to run as NET6.0. Preprocess directives are only looking for NET6_0_OR_GREATER making it very easy to change to NET6+.

Compiler recommend converting delegate variable...

Func<object?[], Task<object?>> invoker = async args =>

...to a local and async function.

async Task<object?> Invoker(object?[] args)

and I fully agree. After feedback, this went through the rounds as being compatible.

Why?

Uses an AsyncServiceScope which allows the IAsyncDisposable interface to be invoked when created by CreateAsyncScope from the IServiceProvider.

This is a more modern way to handle IAsyncDisposable classes that are used in Dependency Injection. Without it, you can trigger these types of exceptions.

Unhandled exception. System.InvalidOperationException: 'MyClass' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.

Also handles IDisposable the same as it did before.

Checklist

1. Closes

Should close issue #204

2. Testing

Ran all UnitTests locally to ensure backwards compatibility. Future testing needed when the main Temporalio is brought up to be compatible with NET6 or NET8.

224/224 Passing.
image

3. Any docs updates needed?

I do not believe so. This code follows typical usage as presented by Microsoft internally to AspNetCore/BackgroundServices or through the dotnet/runtime.

Allow the usage of AsyncServiceProvider for NET6+.
Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Just a couple of other parts I was concerned about, but I like the approach in general (sorry for the run-around, thanks for sticking with it)

@cretz
Copy link
Member

cretz commented Mar 26, 2024

Thanks again!

@cretz cretz merged commit c6b7aca into temporalio:main Mar 26, 2024
6 checks passed
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.

None yet

2 participants