AspNetCoreOData

AspNetCoreOData copied to clipboard

Hi,

My team and I have noticed that when hitting an OData endpoint in parallel, the results returned seem to get scrambled, as though there is some kind of cross-talk between the responses.

The various outcomes we see are;

• Everything is fine.
• Response is a 200 OK, but content is incomplete.
• Response is a 200 OK, but content is empty.
• Responses is a 200 OK, but content contains duplication, or bits of another response.
• Response is a 500 ERROR, and content is incomplete.

I have an integration test showing the failure in the repo here; https://github.com/RobTF/AspNetCoreODataExamples/tree/master/StressTest

This seems like quite a major issue unless we are doing something drastically wrong in the setup. This seems to happen with even 2-3 concurrent calls to the server, so it's not a volume thing.

thanks,

As an FYI this is likely the same as #57 and #104 and related to #108 (which I just opened)

Thanks @Greg-Hitchon - I hadn't noticed this had been reported before. This is probably the most urgent issue at the moment as it's a security issue when response outputs bleed into each other.

Hi @RobTF,

We use Automapper and OData in our project, and this can bring some problems on $filter that OData used alone doesn't have for example. Currently, OData is tested with the latest nightly build with concurrency in our project and no such problems as you wrote happen, but I mentioned that our OData controller functions are simpler than yours, for example Get(key)

I checked your project and I rewrote your Get function of AccountsController: Your Get: public async Task<IActionResult> Get(Guid key, ODataQueryOptions<Account> options) { var query = _accountRepository.Get().Where(x => x.Id == key);

        var finalQuery = options.ApplyTo(query.ProjectTo<Account>(_mapper.ConfigurationProvider)) as IQueryable<dynamic>;
        var x = await finalQuery.ToArrayAsync();
        var item = x.FirstOrDefault(); // await finalQuery.FirstOrDefaultAsync();
        return item == null ? NotFound() : Ok(item);
    }

Our Get as we use in our project:

        [HttpGet]
        [EnableQuery]
        public async Task<SingleResult<Account>> Get([FromODataUri] Guid key)
        {
            return await Task.Run(() => SingleResult.Create(_mapper.ProjectTo<Account>(_accountRepository.Get().Where(x => x.Id == key))));
        }

@RobTF @Greg-Hitchon I think this is fixed at https://github.com/OData/AspNetCoreOData/pull/72 and included in beta version.

Hi @xuzhg,

I've tried this with both 8.0.0-beta and 8.0.0-Nightly202103051314 (the last nightly prior to the routing attribute changes).

Both still fail the test, so unfortunately this doesn't appear fixed, or this is a different issue.

@magneticcore, thanks for your alternate approach. However your example is for an endpoint not related to the failure - the controller action failing in the test is the GetUsersInAccount method which is returning a collection as opposed to a single object. If you have an alternate for that method that makes things work in the failing test that would be great to see!

I've tried various permutations of using [EnableQuery] and returning an IQueryable vs. using ODataQueryOptions and returning IActionResult but nothing seems to make a difference to the result of the test.

I've been doing some more digging; there is something fishy going on in ODataResourceSerSerializer.cs when it calls messageWriter.CreateODataResourceSetWriterAsync. The writer returned from this method can seemingly come back with a stream from a different request.

Perhaps this indicates a bug in the core OData libraries?

In the screenshot below, I fired off two concurrent requests, each with a different query string value (0 or 1). I then trace the 0 and 1 across the calls - tagging the response streams and seeing what happens - there definitely appears to be something happening that looks odd.

Ok, it looks like part of the process used for creating a writer in the OData library gets an instance of ODataMessageInfo from the DI container. This is an implementation detail of the core OData library. The ASP.NET Core OData library appears to feed both calls the same DI container instance and they stomp over the message info object (which has a property containing the stream!!).

I think the bug is possibly in how the ASP.NET Core OData Lib deals with instances of service provider. There does appear to be a bunch of trickery going on, such as creating a ServiceProvider instance per EDM model type etc, looking at the DI services the vast majority are Singleton which may be why it was deemed safe.

I'll see if I can learn a bit more about how the DI stuff works w.r.t. ASP.NET Core OData.

Ok, interestingly it seems as though for batched requests a DI scope is created per logical request, but for non-batched requests the root DI container for the EDM type is used - this is where the problem lies as all non batched requests will share a scope.

If I apply the following hack to force the OData library to create a per-request scope in my apps Configure method my unit tests pass fine, even when running hundreds of concurrent requests. I would not class this as a true fix, however - so please don't use this in production!!

I don't know enough about the library (yet) to put together a formal PR with a proper fix, I'll take a look when I have a moment but hopefully even if I don't manage to get to it, there should be enough detail in this thread for someone more familiar with the project to fix it.

        public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            if (env.IsDevelopment())
            {
                app.UseDeveloperExceptionPage();
            }

            app.UseRouting();

            // ---------------------------------
            // HACK: Force an OData scoped service provider to fix concurrency issues.
            app.Use(next => context =>
            {
                context.Request.CreateSubServiceProvider(context.Request.ODataFeature().PrefixName);
                return next(context);
            });
            // ---------------------------------

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapControllers();
            });
        }

Hi,

I've had a stab at a PR to fix this - #111

I've added the per-request scoping as a middleware. This means that when using OData you will always have to call app.UseOData() to add the middleware into your pipeline, optionally with a parameter to enable batching. I can't see an alternative to this approach without some more major brain surgery.

There was also an issue with the query validator which counted the maximum number of query nodes - the field used for counting the nodes was in a singleton which caused problems, I have switched this to scoped.

Interestingly, there was already a test for query concurrency in the code base which ran 100 concurrent queries but was commented out as it was suggested it simply "didn't work". I've re-enabled this test and it now passes OK with my changes.

Hope this is helpful - it was a somewhat selfish endeavour as I'm desperate to get this fixed otherwise it will be a blocker for my team and I ;)

Hi,

I've had a stab at a PR to fix this - #111

I've added the per-request scoping as a middleware. This means that when using OData you will always have to call app.UseOData() to add the middleware into your pipeline, optionally with a parameter to enable batching. I can't see an alternative to this approach without some more major brain surgery.

There was also an issue with the query validator which counted the maximum number of query nodes - the field used for counting the nodes was in a singleton which caused problems, I have switched this to scoped.

Interestingly, there was already a test for query concurrency in the code base which ran 100 concurrent queries but was commented out as it was suggested it simply "didn't work". I've re-enabled this test and it now passes OK with my changes.

Hope this is helpful - it was a somewhat selfish endeavour as I'm desperate to get this fixed otherwise it will be a blocker for my team and I ;)

Great effort! We too are pretty interested in getting this fixed but did not take the extra step in actually fixing it! Lol @ "interestingly"

@RobTF @Greg-Hitchon I think that's related to https://github.com/OData/AspNetCoreOData/blob/master/src/Microsoft.AspNetCore.OData/Extensions/HttpRequestExtensions.cs#L263-L264

@RobTF I reviewed your PR, but to call "UseOData()" is not acceptable.

Do you mind to change your PR based on the "Scope"?

@RobTF Would you please help test the nightly: Microsoft.AspNetCore.OData.8.0.0-Nightly202103172053.nupkg

@xuzhg - thanks for the reply! I thought of making the change in the place you suggested instead of an app.UseOData() call, but there were two things that stopped me;

1. The method in question is GetSubServiceProvider which looks to potentially be called multiple times per request to get the scope. Simply adding a create call into this method may cause a single request to generate multiple scopes each time the scope is fetched won't it? At the very least there would need to be a guard preventing this.

2. It wasn't clear how one would clean up a scope at the end of the request without a middleware to perform such clean-up.

I see you've made changes to the PR/code so I'll take a look - maybe you've answered these for me already :)

Hi @xuzhg, any thoughts on my points above? I'm looking to test this change next week.

thanks

1. The method in question is GetSubServiceProvider which looks to potentially be called multiple times per request to get the scope. Simply adding a create call into this method may cause a single request to generate multiple scopes each time the scope is fetched won't it? At the very least there would need to be a guard preventing this.

The sub service provider is stored in the ODataFeature once it's created for each request. Would you please point me where do you see the multiple scopes created?

It wasn't clear how one would clean up a scope at the end of the request without a middleware to perform such clean-up.

By default, it's controlled by the scope the request. But, There's also an extension method "DeleteSubServiceProvider" to call, right?

Hi there,

I just thought I'd post a workaround we found for this, as we are currently on 7.5.4 version to be able to get all the various libraries to work together (swagger + swaggerui + versioning) and can't transition to 8 atm.

After an inordinate amount of time, we've realized that this only happens if there is an [EnableQuery] active on the method. Instead of using the [EnableQuery] and the no param Get() method, use this method signature, obviously without the [EnableQuery] attribute.

public IActionResult Get(ODataQueryOptions<Transaction> queryOptions)
{
	return Ok(queryOptions.ApplyTo(DbContext.Transactions));
}

This seems to work and not return random data when using parallel service calls. It's not perfect, but it is a workaround.

@All, I published a RC version at nuget.org. you can download at https://www.nuget.org/packages/Microsoft.AspNetCore.OData/8.0.0-rc

For the currency request, I found an issue (it could be the root cause) and fixed it at https://github.com/OData/AspNetCoreOData/commit/b65d7e997c11f179d4dbdde6a7e62f9925cea01f, and shipped in RC.

@RobTF, @DawidPotgieter @Greg-Hitchon Would you please check it?

Please let me know any other questions.

Hi @xuzhg,

Great stuff on the RC thank you! The issue in b65d7e9 isn't the root cause, I had identified and fixed that one in my PR (and mentioned it here) already but it only causes a failure on the query validation, and does not cause response crosstalk.

The underlying issue with the request mixup was the other stuff I mentioned related to ODataMessageInfo which you fixed due to adding a call to CreateSubServiceProvider inside of GetSubServiceProvider. I'm still not entirely convinced this approach won't create multiple scopes per request though. As each call to the get method in turn calls create and there appears to be nothing preventing it creating a new scope each time.

@RobTF Was the issue resolved?