scottoffen / grapevine-legacy

Embedding A REST Server In Your Application Should Be Simple
http://scottoffen.github.io/grapevine-legacy/
Apache License 2.0
209 stars 51 forks source link

Managing HttpListenerException On Closed Connections #177

Closed napiro closed 7 years ago

napiro commented 7 years ago

Hi

Today I've encountered an exception which can be reproduced reliably with version 4.1.

After a second an exception is thrown in class HttpResponse:

An exception of type 'System.Net.HttpListenerException' occurred in System.dll but was not handled in user code. Additional information: The specified network name is no longer available

The problem seems to be that the client closes the connection while the server tries to write data to the stream which fails. The following line is responsible for the exception:

public void SendResponse(byte[] contents) ... Response.OutputStream.Write(contents, 0, contents.Length);

Is this something you are aware of? Is this something you want to make transparent from the framework point of view and should be handled outside of Grapevine or is it a bug which should be fixed?

Currently I just ignore this error.

Thanks

BTW: I like this project very much!

ghost commented 7 years ago

Hi,

I also ran into this problem. Here is what I got from research:

in Router.Route(IHttpContext context, IList routing)

in Router.Route(object state)

if (e is NotFoundException) { context.Response.SendResponse(HttpStatusCode.NotFound, e.Message); return; }

I don't see any workaround for that behavior, because even general exception handling by intercepting Application.ThreadException and AppDomain.CurrentDomain.UnhandledException is unable to catch the exception that occurs on Response.ContentLength64 = contents.Length;

As a temporary fix I will include the Grapevine project directly into my solution and will modify the code so that it won't treat the !context.WasRespondedTo as an exceptional condition. I hope that Mr. Scott Offen will implement a more meaningful fix in the next versions of the framework.

Thank you for Grapevine, Scott! Daniel

ghost commented 7 years ago

P.S. I believe the problem is here:

public bool WasRespondedTo => Response.ResponseSent;

Response.ResponseSent is not semantically equivalent to WasRespondedTo. WasRespondedTo must be set when at least one route has been found that can handle the request; Response.ResponseSent indicates that the entire response has been sent, which may not be the case if the client breaks the connection, and yet, the server has done everything correctly, and no exception must be thrown. I think, there are actually 3 different indicators during responding in Grapevine:

ghost commented 7 years ago

Here is a temporary fix:

in HttpResponse.cs

    public void SendResponse(byte[] contents)
    {
        if (RequestHeaders.AllKeys.Contains("Accept-Encoding") && RequestHeaders["Accept-Encoding"].Contains("gzip") && contents.Length > 1024)
        {
            using (var ms = new MemoryStream())
            {
                using (var zip = new GZipStream(ms, CompressionMode.Compress))
                {
                    zip.Write(contents, 0, contents.Length);
                }
                contents = ms.ToArray();
            }
            Response.Headers["Content-Encoding"] = "gzip";
        }

        try
        {
            Response.ContentLength64 = contents.Length;
            Response.OutputStream.Write(contents, 0, contents.Length);
            Response.OutputStream.Close();
            Advanced.Close();
        }
        catch (HttpListenerException ex)
        {
            switch (ex.NativeErrorCode)
            {
                //  ERROR_NETNAME_DELETED, https://msdn.microsoft.com/en-us/library/windows/desktop/ms681382(v=vs.85).aspx
                case 64:
                    Console.WriteLine("Connection aborted during SendResponse.");
                    break;
                default:
                    Console.WriteLine(ex.ToString());
                    break;
            }
        }
    }

in Router.cs

    public void Route(IHttpContext context, IList<IRoute> routing)
    {
        if (routing == null || !routing.Any()) throw new RouteNotFoundException(context);
        var totalRoutes = routing.Count;
        var routeCounter = 0;

        Logger.BeginRouting($"{context.Request.Id} - {context.Request.Name} has {totalRoutes} routes");

        bool routeFound = false;
        try
        {
            OnBeforeRouting(context);

            foreach (var route in routing.Where(route => route.Enabled))
            {
                routeFound = true;
                routeCounter++;
                route.Invoke(context);

                Logger.RouteInvoked($"{context.Request.Id} - {routeCounter}/{totalRoutes} {route.Name}");

                if (ContinueRoutingAfterResponseSent) continue;
                if (context.WasRespondedTo) break;
            }
        }
        finally
        {
            OnAfterRouting(context);
            Logger.EndRouting($"{context.Request.Id} - {routeCounter} of {totalRoutes} routes invoked");
        }

        if (!routeFound) throw new RouteNotFoundException(context);
    }
scottoffen commented 7 years ago

I've implemented a fix to this, based in part by your suggested fix.

First, in the SendResponse method, I wrapped writing to the response in a try/finally block:

try
{
    Response.ContentLength64 = contents.Length;
    Response.OutputStream.Write(contents, 0, contents.Length);
}
finally
{
    Response.OutputStream.Close();
    Advanced.Close();
}

This ensures that even if an exception was thrown, the HttpResponse.ResponseSent property still gets set, which is what the HttpContext.WasRespondedTo bool keys off of, and therefore prevent the RouteNotFoundException from being called, which causes further exceptions to be thrown

It will also throw the exception back up to the router, so I added a catch block to the existing try/finally block that was already there:

catch (HttpListenerException e)
{
    var msg = e.NativeErrorCode == 64
        ? "Connection aborted by client"
        : "An error occured while attempting to respond to the request";
    Logger.Error(msg, e);
}

This logs to exception with a message appropriate to the error that occurred, and the server continues running without problem.

Thoughs?

scottoffen commented 7 years ago

Response.ResponseSent is not semantically equivalent to WasRespondedTo. WasRespondedTo must be set when at least one route has been found that can handle the request; Response.ResponseSent indicates that the entire response has been sent, which may not be the case if the client breaks the connection, and yet, the server has done everything correctly, and no exception must be thrown.

This (emphasis mine) isn't entirely correct.

I anticipate that multiple routes may be found that should be invoked on the context, and iterate over them until one of them actually sends a response (default behavior, a flag can override this). Even if code is executed, unless at least one route attempts close the response, the request has not been responded to. This can be done in one of two ways:

  1. Calls to SendResponse(byte[] contents) will automatically close the response

  2. Close the response w/o actually sending anything by calling HttpContext.HttpResponse.Advanced.Close()

It is the closing of the response that indicates whether the request was appropriately responded to.