improvement: Add cancellation support for long running queries

[rpallares]

Hi,

I open this PR to support cancellation for long running queries.
It's inpired from #423.

The root problem is related to PageSize feature that imply synchornous queries.
This force us to not use it and expose directly IAsyncEnumerable.
The counterpart is clients are able to totally dump our database, and if not wanted, it's still nice to cancel thé request.
That issue force to not apply Security guidance for ASP.NET Core Web API OData suggestions to not sacrifice performance.

To reduce a bit risks a bit cancellation must be supported to at least reduce ressource consumption when the client timeout or cancel on long running queries.

The PageSize issue should be also fixed.

[rpallares]

I'm not sure how I could test that but I'm open to suggestions

[rpallares]

@microsoft-github-policy-service agree

[habbes]

@rpallares could you create a separate issue describing the PageSize issue in detail?

[rpallares]

@rpallares could you create a separate issue describing the PageSize issue in detail?

Hi, I'm note sure it's necessary to create a new one because all these existing issues already details that comportement:

• PageSize attribute causes significant drop in throughput #847
• TruncatedCollection does not implement IAsyncEnumerable #253
• Cannot avoid using synchronous calls in OData with entity framework #1194

There's probably more related as #423 seems to come from very old issue and even older pr #324

I also saw references inside https://github.com/OData/WebApi

• Returning an IQueryable from EF core 6 generates synchronous database calls WebApi#2598
• ODataQueryOptions forces materialization of the IQueryable when PageSize is set WebApi#886

If any brealing change is required, odata team should really consider add it in the next major version.

Thanks

[habbes]

@rpallares

I'm not sure how I could test that but I'm open to suggestions

Here's a suggestion of how you could test this. The general idea is to pass a cancellation token for which you have the CancellationTokenSource, then when you run the operation you want to abort, cancel the token and assert that an OperationCanceledException was thrown (assuming you're handling the cancellation by throwing that exception).

Look at the ODataResourceSetSerializerTests for examples of tests targeting the ODataResourceSetSerializer.

First, I think it will be easier to create unit tests if the ODataSerializerContext.CancellationToken property is settable. You can set it to the Request.HttpContext.RequestAborted token in the constructor, or if it's unset, but I think it would be easier not to fully couple it to the request.

Create a test async enumerable data source that will generate the sample data to serialize and will handle the cancellation token that will be passed by the serializer:

private static async IAsyncEnumerable<Address> GetAsyncAddressesDataSource(
    [EnumeratorCancellation] CancellationTolen cancellationToken = default)
{
    for (int i = 0; i < 1000; i++)
    {
        yield return new Address { City = "City" };
        await Task.Yield();
        cancellationToken.ThrowIfCancellationRequested();
    }
}

Ideally the enumeration will be cancelled before it runs through all those iterations. If we abort the request before we start serializing, then we're guaranteed the enumeration will be cancelled (if cancellation token is propagated correctly). If we cancel the token concurrently with serialization, it's possible it might run through the whole loop before cancellation is triggered even if the code is correct. So this could be a flaky test if we do it that way, but the Task.Yield() and the large number of iterations aim to avoid that. We could also use a while (true) but that would hang the test instead of failing if cancellation is not triggered.

Now we'll need a way to create trigger the cancellation of the token from the test. One way to do this would be to create an instance of CancellationTokenSource then pass its token to the ODataSerializerContext.Token property. However, in your PR, this property is read-only, and gets the token from the request. We can easily get around this limitation by making the ODataSerializerContext.Token property settable. So we can override the token in the test. That would work fine, but would leave a gap in the test, in that we won't be verifying that it's the cancellation of the HttpRequest the serialization to cancel. Maybe we could test the follow of the request's cancellation token to the serializer context separately to close this gap.

Alternatively, we could inject an override the cancellation token in the Request.HttpContext. This is a bit more involved, but I think it's worth it.

First, we use the RequestFactory.Create() helper to create a new request. We could simply pass this request to the serializer context in the test. And our test code would look like so:

var request = RequestFactory.Create();

// ...
var addresses = GetAsyncAddressesDataSource();
var type = typeof(IAsyncEnumerable<Address>);
ODataSerializationContext writeContext = new ODataSerializationContext
{
    Model = model,
    Type = type ,
    Request = request
};

// in this example we abort before we run serialization
writeContext.Request.HttpContext.Abort();
await Assert.ThrowAsync<OperationCanceledException>(() => serializer.WriteObjectAsync(addresses, type, writer, writeContext);

In the example above we abort the request before serialization starts, so we're guaranteed the enumeration will be cancelled if the code is implemented correctly. We could also abort after the serialization has started, which I think is more representative of real-world use cases. While technically flaky, I think that's unlikely to manifest in practice given the large number of iterations in the loop:

var serializationTask = serializer.WriteObjectAsync(addresses, type, writer, writeContext);
request.HttpContext.Abort();
await Assert.ThrowsAsync<OperationCanceledException>(() => serializationTask);

This test will fail because this test HttpContext doesn't actually abort anything. We have to override this.
Here's the current implementation of RequestFactory.Create:

public static HttpRequest Create()
{
    HttpContext context = new DefaultHttpContext();
    return context.Request;
}

I've looked at the implementation of DefaultHttpContext and found that you can customize its behaviour by injecting custom "features". To override the cancellation token, we can customize the IHttpRequestLifetimeFeature by implementing our own like so:

private class TestRequestLifeTimeFeature : IHttpRequestLifetimeFeature
{
    private CancellationTokenSource _cts = new CancellationTokenSource();

    public CancellationToken RequestAborted { get => _cts.Token; set => throw new NotImplementedException(); }

    public void Abort()
    {
        _cts.Cancel();
    }
}

This could be an inner helper class inside RequestFactory.

And finally, we can update the request factory to inject this feature:

public static HttpRequest Create()
{
    FeatureCollection features = new();
    features.Set<IHttpRequestLifetimeFeature>(new TestHttpRequestLifetimeFeature());
    HttpContext context = new DefaultHttpContext(features);
    return context.Request;
}

[WanjohiSammy]

@rpallares

Please add the tests like the ones suggested by @habbes above #1271 (comment)

[rpallares]

@rpallares

Please add the tests like the ones suggested by @habbes above #1271 (comment)

Hi, yes. @habbes gave great very detailled suggestions.
Sadly I don't have time to do it quickly.
Don't hesitate to propose pr either. I really believe the two issues pointes here are critical

[rpallares]

I changed a bit against @habbes suggested to be able to correctly dispose the CancellationTokenSource.
I believe that WE should refactor a bit some of RequestFactory Create method to share the base Create method that now take an optional cancellationToken. But it imply much more changes not used yet, only for testing. So I left as is.
I also target the main branch, a cherry pick will be welcome for the 8.x branch (no conflict expected).

[rpallares]

@rpallares could you create a separate issue describing the PageSize issue in detail?

Hi, I'm note sure it's necessary to create a new one because all these existing issues already details that comportement:

* [PageSize attribute causes significant drop in throughput #847](https://github.com/OData/AspNetCoreOData/issues/847)

* [TruncatedCollection does not implement IAsyncEnumerable #253](https://github.com/OData/AspNetCoreOData/issues/253)

* [Cannot avoid using synchronous calls in OData with entity framework #1194](https://github.com/OData/AspNetCoreOData/issues/1194)

There's probably more related as #423 seems to come from very old issue and even older pr #324

I also saw references inside https://github.com/OData/WebApi

* [Returning an IQueryable from EF core 6 generates synchronous database calls WebApi#2598](https://github.com/OData/WebApi/issues/2598)

* [ODataQueryOptions forces materialization of the IQueryable when PageSize is set WebApi#886](https://github.com/OData/WebApi/issues/886)

If any brealing change is required, odata team should really consider add it in the next major version.

Thanks

@habbes , any official news regarding the pagesize topic?
I know you were pretty busy with the major release, but this issue IS really killing scaling performances despite it's a great feature.

[habbes]

I changed a bit against @habbes suggested to be able to correctly dispose the CancellationTokenSource. I believe that WE should refactor a bit some of RequestFactory Create method to share the base Create method that now take an optional cancellationToken. But it imply much more changes not used yet, only for testing. So I left as is. I also target the main branch, a cherry pick will be welcome for the 8.x branch (no conflict expected).

The change looks good to me, simpler and requires less changes.

[habbes]

@rpallares I agree that the page size issue is longstanding issue that we should finally solve. We are looking to invest in more perf improvements to the library and this is one that should definitely be part of that. I can't give an ETA yet, but I'm bringing this to the attention of the team. We also want to get rid of places where synchronous I/O is forced even when the data source is async, and this is one of such bottlenecks.

[gathogojr]

Thank you @rpallares for your contribution