Permalink
Find file Copy path
0ce9242 Jan 23, 2019
3 contributors

Users who have contributed to this file

@davidfowl @Tornhoof @sandorfr
419 lines (326 sloc) 15.4 KB

Table of contents

ASP.NET Core Guidance

ASP.NET Core is a cross-platform, high-performance, open-source framework for building modern, cloud-based, Internet-connected applications. This guide captures some of the common pitfalls and practices when writing scalable server applications.

Avoid using synchronous Read/Write overloads on HttpRequest.Body and HttpResponse.Body

All IO in ASP.NET Core is asynchronous. Servers implement the Stream interface which has both synchronous and asynchronous overloads. The asynchronous ones should be preferred to avoid blocking thread pool threads (this could lead to thread pool starvation).

BAD This example uses the StreamReader.ReadToEnd and as a result blocks the current thread to wait for the result. This is an example of sync over async.

public class MyController : Controller
{
    [HttpGet("/pokemon")]
    public ActionResult<PokemonData> Get()
    {
        // This synchronously reads the entire http request body into memory.
        // If the client is slowly uploading, we're doing sync over async because Kestrel does *NOT* support synchronous reads.
        var json = new StreamReader(Request.Body).ReadToEnd();

        return JsonConvert.DeserializeObject<PokemonData>(json);
    }
}

GOOD This example uses StreamReader.ReadToEndAsync and as a result, does not block the thread while reading.

public class MyController : Controller
{
    [HttpGet("/pokemon")]
    public async Task<ActionResult<PokemonData>> Get()
    {
        // This synchronously reads the entire http request body into memory.
        var json = await new StreamReader(Request.Body).ReadToEndAsync();

        return JsonConvert.DeserializeObject<PokemonData>(json);
    }
}

💡NOTE: If the request is large it could lead to out of memory problems which can result in a Denial Of Service. See this for more information.

Prefer using HttpRequest.ReadAsFormAsync() over HttpRequest.Form

You should always prefer HttpRequest.ReadAsFormAsync() over HttpRequest.Form. The only time it is safe to use HttpRequest.Form is the form has already been read by a call to HttpRequest.ReadAsFormAsync() and the cached form value is being read using HttpRequest.Form.

BAD This example uses HttpRequest.Form uses sync over async under the covers and can lead to thread pool starvation (in some cases).

public class MyController : Controller
{
    [HttpGet("/form-body")]
    public IActionResult Post()
    {
        var form = HttpRequest.Form;
        
        Process(form["id"], form["name"]);

        return Accepted();
    }
}

GOOD This example uses HttpRequest.ReadAsFormAsync() to read the form body asynchronously.

public class MyController : Controller
{
    [HttpGet("/form-body")]
    public async Task<IActionResult> Post()
    {
        var form = await HttpRequest.ReadAsFormAsync();
        
        Process(form["id"], form["name"]);

        return Accepted();
    }
}

Use buffering and synchronous reads and writes as an alternative to asynchronous reading and writing

TBD

Avoid reading the entire request body or response body into memory

TBD

Do not store IHttpContextAccessor.HttpContext in a field

The IHttpContextAccessor.HttpContext will return the HttpContext of the active request when accessed from the request thread. It should not be stored in a field or variable.

BAD This example stores the HttpContext in a field then attempts to use it later.

public class MyType
{
    private readonly HttpContext _context;
    public MyType(IHttpContextAccessor accessor)
    {
        _context = accessor.HttpContext;
    }
    
    public void CheckAdmin()
    {
        if (!_context.User.IsInRole("admin"))
        {
            throw new UnauthorizedAccessException("The current user isn't an admin");
        }
    }
}

The above logic will likely capture a null or bogus HttpContext in the constructor for later use.

GOOD This example stores the IHttpContextAccesor itself in a field and uses the HttpContext field at the correct time (checking for null).

public class MyType
{
    private readonly IHttpContextAccessor _accessor;
    public MyType(IHttpContextAccessor accessor)
    {
        _accessor = accessor;
    }
    
    public void CheckAdmin()
    {
        var context = _accessor.HttpContext;
        if (context != null && !context.User.IsInRole("admin"))
        {
            throw new UnauthorizedAccessException("The current user isn't an admin");
        }
    }
}

Do not access the HttpContext from multiple threads in parallel. It is not thread safe.

The HttpContext is NOT threadsafe. Accessing it from multiple threads in parallel can cause corruption resulting in undefined behavior (hangs, crashes, data corruption).

BAD This example makes 3 parallel requests and logs the incoming request path before and after the outgoing http request. This accesses the request path from multiple threads potentially in parallel.

public class AsyncController : Controller
{
    [HttpGet("/search")]
    public async Task<SearchResults> Get(string query)
    {
        var query1 = SearchAsync(SearchEngine.Google, query);
        var query2 = SearchAsync(SearchEngine.Bing, query);
        var query3 = SearchAsync(SearchEngine.DuckDuckGo, query);

        await Task.WhenAll(query1, query2, query3);
        
        var results1 = await query1;
        var results2 = await query2;
        var results3 = await query3;

        return SearchResults.Combine(results1, results2, results3);
    }

    private async Task<SearchResults> SearchAsync(SearchEngine engine, string query)
    {
        var searchResults = SearchResults.Empty;
        try
        {
            _logger.LogInformation("Starting search query from {path}.", HttpContext.Request.Path);
            searchResults = await _searchService.SearchAsync(engine, query);
            _logger.LogInformation("Finishing search query from {path}.", HttpContext.Request.Path);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, "Failed query from {path}", HttpContext.Request.Path);
        }

        return searchResults;
    }
}

GOOD This example copies all data from the incoming request before making the 3 parallel requests.

public class AsyncController : Controller
{
    [HttpGet("/search")]
    public async Task<SearchResults> Get(string query)
    {
        string path = HttpContext.Request.Path;
        var query1 = SearchAsync(SearchEngine.Google, query, path);
        var query2 = SearchAsync(SearchEngine.Bing, query, path);
        var query3 = SearchAsync(SearchEngine.DuckDuckGo, query, path);

        await Task.WhenAll(query1, query2, query3);
        
        var results1 = await query1;
        var results2 = await query2;
        var results3 = await query3;

        return SearchResults.Combine(results1, results2, results3);
    }

    private async Task<SearchResults> SearchAsync(SearchEngine engine, string query, string path)
    {
        var searchResults = SearchResults.Empty;
        try
        {
            _logger.LogInformation("Starting search query from {path}.", path);
            searchResults = await _searchService.SearchAsync(engine, query);
            _logger.LogInformation("Finishing search query from {path}.", path);
        }
        catch (Exception ex)
        {
            _logger.LogError(ex, "Failed query from {path}", path);
        }

        return searchResults;
    }
}

Do not use the HttpContext after the request is complete

The HttpContext is only valid as long as there is an active http request in flight. The entire ASP.NET Core pipeline is an asynchronous chain of delegates that executes every request. When the Task returned from this chain completes, the HttpContext is recycled.

BAD This example uses async void (which is a ALWAYS bad in ASP.NET Core applications) and as a result, accesses the HttpResponse after the http request is complete. It will crash the process as a result.

public class AsyncVoidController : Controller
{
    [HttpGet("/async")]
    public async void Get()
    {
        await Task.Delay(1000);

        // THIS will crash the process since we're writing after the response has completed on a background thread
        await Response.WriteAsync("Hello World");
    }
}

GOOD This example returns a Task to the framework so the http request doesn't complete until the entire action completes.

public class AsyncController : Controller
{
    [HttpGet("/async")]
    public async Task Get()
    {
        await Task.Delay(1000);
        
        await Response.WriteAsync("Hello World");
    }
}

Do not capture the HttpContext in background threads

BAD This example shows a closure is capturing the HttpContext from the Controller property. This is bad because this work item could run outside of the request scope and as a result, could lead to reading a bogus HttpContext.

[HttpGet("/fire-and-forget-1")]
public IActionResult FireAndForget1()
{
    _ = Task.Run(() =>
    {
        await Task.Delay(1000);

        // This closure is capturing the context from the Controller property. This is bad because this work item could run
        // outside of the http request leading to reading of bogus data.
        var path = HttpContext.Request.Path;
        Log(path);
    });

    return Accepted();
}

GOOD This example copies the data required in the background task during the request explictly and does not reference anything from the controller itself.

[HttpGet("/fire-and-forget-3")]
public IActionResult FireAndForget3()
{
    string path = HttpContext.Request.Path;
    _ = Task.Run(async () =>
    {
        await Task.Delay(1000);

        // This captures just the path
        Log(path);
    });

    return Accepted();
}

Do not capture services injected into the controllers on background threads

BAD This example shows a closure is capturing the DbContext from the Controller action parameter. This is bad because this work item could run outside of the request scope and the PokemonDbContext is scoped to the request. As a result, this will end up with an ObjectDisposedException.

[HttpGet("/fire-and-forget-1")]
public IActionResult FireAndForget1([FromServices]PokemonDbContext context)
{
    _ = Task.Run(() =>
    {
        await Task.Delay(1000);

        // This closure is capturing the context from the Controller action parameter. This is bad because this work item could run
        // outside of the request scope and the PokemonDbContext is scoped to the request. As a result, this throw an ObjectDisposedException
        context.Pokemon.Add(new Pokemon());
        await context.SaveChangesAsync();
    });

    return Accepted();
}

GOOD This example injects an IServiceScopeFactory and creates a new dependency injection scope in the background thread and does not reference anything from the controller itself.

[HttpGet("/fire-and-forget-3")]
public IActionResult FireAndForget3([FromServices]IServiceScopeFactory serviceScopeFactory)
{
    // This version of fire and forget adds some exception handling. We're also no longer capturing the PokemonDbContext from the incoming request.
    // Instead, we're injecting an IServiceScopeFactory (which is a singleton) in order to create a scope in the background work item.
    _ = Task.Run(async () =>
    {
        await Task.Delay(1000);

        // Create a scope for the lifetime of the background operation and resolve services from it
        using (var scope = serviceScopeFactory.CreateScope())
        {
            // This will a PokemonDbContext from the correct scope and the operation will succeed
            var context = scope.ServiceProvider.GetRequiredService<PokemonDbContext>();

            context.Pokemon.Add(new Pokemon());
            await context.SaveChangesAsync();
        }
    });

    return Accepted();
}

Avoid adding headers after the HttpResponse has started

ASP.NET Core does not buffer the http response body. This means that the very first time the response is written, the headers are sent along with that chunk of the body to the client. When this happens, it's no longer possible to change response headers.

BAD This logic tries to add response headers after the response has already started.

app.Use(async (next, context) =>
{
    await context.Response.WriteAsync("Hello ");
    
    await next();
    
    // This may fail if next() already wrote to the response
    context.Response.Headers["test"] = "value";    
});

GOOD This example checks if the http response has started before writing to the body.

app.Use(async (next, context) =>
{
    await context.Response.WriteAsync("Hello ");
    
    await next();
    
    // Check if the response has already started before adding header and writing
    if (!context.Response.HasStarted)
    {
        context.Response.Headers["test"] = "value";
    }
});

GOOD This examples uses HttpResponse.OnStarting to set the headers before the response headers are flushed to the client.

It allows you to register a callback that will be invoked just before response headers are written to the client. It gives you the ability to append or override headers just in time, without requiring knowledge of the next middleware in the pipeline.

app.Use(async (next, context) =>
{
    // Wire up the callback that will fire just before the response headers are sent to the client.
    context.Response.OnStarting(() => 
    {       
        context.Response.Headers["someheader"] = "somevalue"; 
        return Task.CompletedTask;
    });
    
    await next();
});