dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.35k stars 9.99k forks source link

httpContext.Response.OnStarting lambdas executing in reverse order #19530

Closed mtamrakar closed 4 years ago

mtamrakar commented 4 years ago

I have a setup where the same cookie might get deleted, set or changed in the same request based on various business logic. As per Microsoft suggestion, I have these cookie updates wrapped up inside OnStarting. In doing so, I have noticed that the lamdas run in the REVERSE order of their registration.

Eg:


            this.HttpContext.Response.OnStarting(
                () =>
                {
                    Console.WriteLine(1);
                    return Task.CompletedTask;
                });

            this.HttpContext.Response.OnStarting(
               () =>
               {
                   Console.WriteLine(2);
                   return Task.CompletedTask;
               });

            this.HttpContext.Response.OnStarting(
               () =>
               {
                   Console.WriteLine(3);
                   return Task.CompletedTask;
               });

            this.HttpContext.Response.OnStarting(
               () =>
               {
                   Console.WriteLine(4);
                   return Task.CompletedTask;
               });

            this.HttpContext.Response.OnStarting(
               () =>
               {
                   Console.WriteLine(5);
                   return Task.CompletedTask;
               });

            this.HttpContext.Response.OnStarting(
               () =>
               {
                   Console.WriteLine(6);
                   return Task.CompletedTask;
               });

            this.HttpContext.Response.OnStarting(
               () =>
               {
                   Console.WriteLine(7);
                   return Task.CompletedTask;
               });

Outputs: 7 6 5 4 3 2 1

ASP.NET Core version: 3.1

mkArtakMSFT commented 4 years ago

@anurse I'm not sure hosting is the right area for this. Can you please adjust as necessary? Thanks!

Tratcher commented 4 years ago

This is intentional to mirror the middleware pipeline ordering. E.g. The last thing to register is the inner most component in the pipeline and so it gets to look at the response first.

Should update the doc comments to reflect this.

That said, what specific guidance did you get for using OnStarting? It's one of the harder APIs to use correctly, especially with complex interactions like this. You might do better to abstract the cookie handling to centralized logic and restrict to a single OnStarting event to apply the aggregate result.

mtamrakar commented 4 years ago

At one point, we were trying solve the "response has started" issue and since then wrapped a cookie manager on top of OnStarting. You can imagine how these type of updates could happen from different part of the code which needs to run in sequence.

 public void DeleteCookie(string name)
        {
            var httpContext = this.httpContextAccessor.HttpContext;

            if (httpContext.Request.Cookies.ContainsKey(name))
            {
                httpContext.Response.OnStarting(
                    () =>
                    {
                        httpContext.Response.Cookies.Delete(
                        name);

                        return Task.CompletedTask;
                    });
            }

Is there any risk of having these updates like this?

 public void DeleteCookie(string name)
        {
            var httpContext = this.httpContextAccessor.HttpContext;
             httpContext.Response.Cookies.Delete(name);
        }
Tratcher commented 4 years ago

Your second example is fine if you check HttpContext.Response.HasStarted first. No cookies can be modified after HasStarted, even with OnStarting.

mtamrakar commented 4 years ago

Ideally, when would we ever do the OnStarting registration then?

Tratcher commented 4 years ago

OnStarting is useful for last second sanity checks, not much else.

mtamrakar commented 4 years ago

Thank you. I'm closing the issue as OnStarting is doing what it supposed to and I do see that a comment update is in progress to clarify it's execution order.

I think we will just go with direct updates and handle out of sequence header updates more carefully.