Breeze / breeze.server.net

Breeze support for .NET servers
MIT License
76 stars 62 forks source link

Add ability to use cancellation tokens with BreezeQueryFilter #191

Closed biegehydra closed 3 months ago

biegehydra commented 1 year ago

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 commented 1 year ago

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

steveschmitt commented 1 year ago

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

biegehydra commented 12 months ago

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 commented 12 months ago

Thanks again. Will review it soon.

steveschmitt commented 12 months ago

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 commented 12 months ago

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

Thanks for doing this, it was very helpful.

papadi commented 3 months ago

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

steveschmitt commented 3 months ago

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

steveschmitt commented 3 months ago

I added it as a separate issue #195