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

Thread-safe GetModelAsync #306

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@rayao
Contributor

rayao commented Mar 7, 2016

This ONLY synchronizes multiple simultaneous calls to model building pipeline, WebApiConfig still could return before a long running GetModelAsync completes.
Partially resolve #304

Thread-safe GetModelAsync
This ONLY synchronizes multiple simultaneous calls to model building pipeline, WebApiConfig still could return before a long running GetModelAsync completes.
var builder = context.GetApiService<IModelBuilder>();
if (builder == null)
{
return null;

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Maybe we should throw if there is no model builder? It is more meaningful than a later null-reference exception.

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

Ok, thanks.
The message I put into Resources is "IEdmModel cannot be generated since API service IModelBuilder is not registered."
Let me know better description.

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

That sounds good:-) Thanks.

@@ -65,5 +68,46 @@ public IServiceProvider ServiceProvider
{
return this.serviceProvider.GetService<T>();
}
internal TaskCompletionSource<IEdmModel> CompeteModelGeneration(out Task<IEdmModel> running)

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Compete or Complete?

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

OK I got that. This should be Compete.

@lewischeng-ms

This comment has been minimized.

Contributor

lewischeng-ms commented Mar 13, 2016

So basically this PR is to make any call to GetModelAsync to wait for the first one to complete? If we need to sync with the later requests, we have to register a model factory to the underlying OData Web API rather than the model instance itself. Is my understanding correct?

source.SetResult(model);
return model;
}
catch (AggregateException e)

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Are we going to do anything special for an AggregateException in the future? If not, we may consider merge the two catch-clauses.

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

If we catch AggregatedException as normal Exception, and SetException to the task, the task will wrap it again and so ends with an AggreatedException, which has an inner AggregatedException, which has the actual exceptions as its InnerExceptions.

using Microsoft.Extensions.DependencyInjection;
using Microsoft.OData.Edm;
using Microsoft.Restier.Core.Properties;

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Thanks for cleaning up unused namespaces!

return null;
}
source.Task.ContinueWith(

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

A naïve question: shall we set the returned task to source.Task otherwise the ContinueWith part will be lost?

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

You mean set the out running parameter? It's not necessary I think, since source.Task continues when SetResult or SetException is called in GetModelAsync, that will trigger the continuation here.

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Sorry I mean Task.ContinueWith returns a continuation task which will be called when, as you said, SetResult or SetException is called. But does it have a side effect that the original task is appended with the continued task as well? If not, source.Task will return the original one rather than the continued one. But that may not be a problem though.

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

Oh yeah, I got your thought. Yes another thread would continue on the original task source task, not the appended one, so it could possibly run before EdmModel is set, but as you pointed out this won't hurt, since anyway the resulting IEdmModel is retrieved.

internal TaskCompletionSource<IEdmModel> CompeteModelGeneration(out Task<IEdmModel> running)
{
var source = new TaskCompletionSource<IEdmModel>(TaskCreationOptions.AttachedToParent);
var exist = Interlocked.CompareExchange(ref modelTask, source.Task, null);

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Shall we name it runningTask to make the code clearer? For example, if we wrote if (runningTask != null), we knew that there is an ongoing task.

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

Thanks.

{
if (task.Status == TaskStatus.RanToCompletion)
{
ReplaceModelTask(Task.FromResult(task.Result), task);

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

So this is to replace the model task with a straightforward task which directly returns the model instance?

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

Partially, the main purpose is to confirm that this is truly the original model generation task. But actually it must be. In a previous version FinishModelGeneration is called from GetModelAsync, so I added this validation.
And I didn't look into Task code but intuitively I think a task built from direct result would consume less resources than a completed task from a completion source.

This comment has been minimized.

@lewischeng-ms

lewischeng-ms Mar 13, 2016

Contributor

Intuitively I think a Task instance primarily contains the id information, state variable, the pointer to a task code. So I would assume that all task instances are of the same size. So it's OK to leave the completed task as is. But I do think the design to replace the modelTask with null to allow retry is very good:)

This comment has been minimized.

@rayao

rayao Mar 13, 2016

Contributor

Removed the replace call and adds an assert.
Debug.Assert(task == modelTask);
Thanks.

@rayao

This comment has been minimized.

Contributor

rayao commented Mar 13, 2016

So basically this PR is to make any call to GetModelAsync to wait for the first one to complete? If we need to sync with the later requests, we have to register a model factory to the underlying OData Web API rather than the model instance itself. Is my understanding correct?

Perhaps I don't get your idea clearly, since GetModelAsync works on ApiConfiguration, so calling it with any model instance will be synchronized. So, if a query needs EdmModel to complete, an await GetModelAsync will guarantee that.
The real problem is the async nature of GetModelAsync, since WebApiConfig.Register is synchronous, it can't wait Restier route to be completely established, for example:

        public static async void RegisterNorthwind(
            HttpConfiguration config, HttpServer server)
        {
            await config.MapRestierRoute<NorthwindApi>(
                "NorthwindApi", "api/Northwind",
                new RestierBatchHandler(server));
        }

RegisterNorthwind could have returned before the route is completely registered. So if the model generation is really costly, RESTier routing will likely get messed up.
You can put an await Task.Delay(5000) before the config.Map call to repro.

Resolve comments.
Throw on IModelBuilder missing.
@rayao

This comment has been minimized.

Owner

rayao commented on src/Microsoft.Restier.Core/ApiConfiguration.cs in 9833e49 Mar 13, 2016

Continue it inline so validation is no more necessary.

@lewischeng-ms

This comment has been minimized.

Contributor

lewischeng-ms commented Mar 14, 2016

@rayao Yeah, I understand your concern about requests coming before the route has been completely registered. But in this case, since the route has not been registered yet. The service could safely return 404 for those requests. And what I was thinking about was that you can't process any OData request without getting the model instance (parse URI or serialization/deserialization). Now the model instance is registered staticly into ODataPathRouteConstraint. If we could register a factory method to produce the model into Web API and let the factory method call await GetModelAsync(), the service could probably wait for model build completion then process the first incoming request.

@lewischeng-ms

This comment has been minimized.

Contributor

lewischeng-ms commented Mar 14, 2016

I've no other questions, please feel free to merge it.

@rayao rayao closed this Mar 14, 2016

@rayao rayao deleted the rayao:model branch Mar 14, 2016

@rayao

This comment has been minimized.

Contributor

rayao commented Mar 14, 2016

But in this case, since the route has not been registered yet. The service could safely return 404 for those requests

@lewischeng-ms This is only true for synchronous long running GetModelAsync call (like all of our existing samples, although they return task but they're synchronous in nature). A truly async GetModelAsync call won't have the chance to registered a route before WebApiConfig pipeline ends, and that results in RESTier routing not established, no further requests could be processed.
You can add an await Task.Delay(1000) in model pipeline to see the effect.

If we could register a factory method to produce the model into Web API and let the factory method call await GetModelAsync()

Yeah I initially thought that, but since the route selection pipeline is synchronous, we can't await.
I've checked in this change and working on another PR which could possibly be the resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment