dotnet/aspnetcore

NotifyOnCompleted does not catch exceptions, may abort the response/connection

Open

#5,885 opened on Mar 7, 2018

View on GitHub
 (3 comments) (0 reactions) (0 assignees)C# (37,933 stars) (10,653 forks)batch import
affected-very-fewarea-networkingbugfeature-httpsyshelp wantedseverity-nice-to-have

Description

NotifyOnCompleted should catch and log its own exceptions as to not disrupt the normal request flow. It doesn't.

https://github.com/aspnet/HttpSysServer/blob/336e85d19abcdf3ec3a024cb9431c768a4e5d494/src/Microsoft.AspNetCore.Server.HttpSys/MessagePump.cs#L204-L214 https://github.com/aspnet/HttpSysServer/blob/336e85d19abcdf3ec3a024cb9431c768a4e5d494/src/Microsoft.AspNetCore.Server.HttpSys/FeatureContext.cs#L586-L591

It also fires before the response is flushed/disposed so these exceptions may abort the response/connection. Compare to https://github.com/aspnet/KestrelHttpServer/pull/2324

Contributor guide

Tech stack
csharp
Domain
backend
Issue type
bug
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
3
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
under 1 hour
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
stale
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
C#ASP.NET Core
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
70
Research direction
Review the referenced code in MessagePump.cs (lines 204-214) and FeatureContext.cs (lines 586-591) to understand how NotifyOnCompleted is invoked. The issue states that exceptions in NotifyOnCompleted are not caught, which can abort the response. Look at the linked KestrelHttpServer pull request #2324 for a similar fix. Implement a try catch block that logs exceptions and ensures the normal request flow is not disrupted.