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

Add ability to use cancellation tokens with BreezeQueryFilter #191

Closed
wants to merge 7 commits into from

Conversation

biegehydra
Copy link
Contributor

@biegehydra biegehydra commented Jul 20, 2023

Fixes issue #189

I think this is the best solution to the problem of cancellation tokens. They should be natively supported because the breeze client natively accepts a cancellation token as a parameter when sending a request. Telling people to use an additional filter/middleware to catch an OperationCanceledException is not only a klunky solution, but it's hardly even a solution.

The problem with that approach is the exception won't be thrown until after ToList() has completed - it's synchronous. This means the entire database query will still run to completion in entirety, making the exception thrown after pointless. The point of a cancellation token is to stop the database query and save resources. Middleware/additional filters can't do that. To do that, you have to pass the cancellation token to ToListAsync(). That function is part of EF so I ported it to this project and made it internal.

I added a variable to the BreezeQueryFilter called CatchCancellations so this is an opt in feature and won't cause any breaking changes.

My solution uses an IAsyncEnumerable which isn't part of netstandard2.0 so I only made my changes in the Dotnet projects. I feel like it should be possible for a solution to be created for AspNetCore but I couldn't find a way to do it.

Testing

I was not able to run the unit tests so this was only tested inside of another project. In my testing, everything works as expected. If the CatchCancellations is enabled, the cancellation is detected in ToListAsync, the process is cancelled, and database resources are saved.

@steveschmitt
Copy link
Member

Thanks, great work! I'll take a look and run some tests.

@steveschmitt
Copy link
Member

For the async code, shouldn't we override OnActionExecutionAsync?

@biegehydra
Copy link
Contributor Author

biegehydra commented Aug 1, 2023

For some reason I was under the impression that IAsyncActionFilters couldn't be used as attributes. Turns out they can, you just inherit Attribute. I went ahead and did the conversion and its working great. Being async also made the code a little cleaner.

This will help all the servers that are using breeze drastically. Previously every db query was being run synchronously and locking up the thread. This will free up the threads while db queries run.

@steveschmitt
Copy link
Member

Thanks again. Will review it soon.

@steveschmitt
Copy link
Member

steveschmitt commented Aug 2, 2023

I'm wondering about the AsAsyncEnumerable() method, which throws if the source is not IAsyncEnumerable. What happens when the BreezeQueryFilter is applied to non-async controller methods?

UPDATE
I tested it, and it throws an exception. Let's change the ToListAsync extension method to:

    internal static async Task<List<TSource>> ToListAsync<TSource>(this IQueryable<TSource> source, CancellationToken cancellationToken = default) {
      if (source is IAsyncEnumerable<TSource> asyncEnumerable) {
        var list = new List<TSource>();
        await foreach (var element in asyncEnumerable.WithCancellation(cancellationToken)) {
          list.Add(element);
        }
        return list;
      } else {
        return source.ToList();
      }
    }

@steveschmitt
Copy link
Member

I've merged the PR, after making the change I described above.

Thanks for doing this, it was very helpful.

@papadi
Copy link

papadi commented Apr 3, 2024

I'm afraid this change removes the option to override the behavior. Previously, OnActionExecuted was overridable before. The methods aren't.

@steveschmitt
Copy link
Member

@papadi That's a very good point! I'll try to fix that.

@steveschmitt
Copy link
Member

I added it as a separate issue #195

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.

3 participants