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 50 forks source link

RestClient.Execute doesn't respect WebExceptionHandler #219

Closed mrabey closed 3 years ago

mrabey commented 3 years ago

Hello,

Before I go into detail about my issue, I am aware that this version of Grapevine (4.2.0) is no longer the latest version, however it is still in maintenance mode. Switching to the latest version is not possible at this time.

To preface this, I am attempting to discover the best way to be alerted to disconnections either from the server going down, or the client losing connection to the network, etc (any of those unforeseen issues that could cause a message send or receive failure). So I have a little test application that creates a RestServer and RestClient communicating over localhost using port 8884.

To my issue, I am looking through the source code for RestClient, specifically the Execute method, seeing how the WebExceptionHandlers dictionary is used. I see that in case an exception is encountered, you first search for the WebExceptionStatus key in the dictionary, and if found, you Invoke that handler and then return null. If the key is not found, you throw that exception to the invoking method (whatever executed the RestClient.Execute(IRestRequest)). However, in my specific case, I'm seeing that this is not the case. Below is a sample of what I'm doing.

public class Client
{
   private RestClient _client;

   public Client(string addr, int port) 
   {
       _client = new RestClient()
       {
           Host = addr,
           Port = port
       };

       _client.WebExceptionHandlers = new System.Collections.Generic.Dictionary<WebExceptionStatus, WebExceptionHandler>
       {
           { WebExceptionStatus.ConnectFailure, GenericExceptionHandler },
           { WebExceptionStatus.ConnectionClosed, GenericExceptionHandler },
           { WebExceptionStatus.Timeout, GenericExceptionHandler },
           { WebExceptionStatus.SendFailure, GenericExceptionHandler }
       };
   }

   private void GenericExceptionHandler(IRestClient client, IRestRequest request, WebException exception)
   {
       Console.WriteLine($"Client ({client.Host}) ran into exception ({exception.Status}) with message:\n" + exception.Message);
   }
}

So, the WebExceptionHandlers dictionary is filled with general coverage of failure points. The issue I'm running into is with the ConnectFailure handler. Here is the execution method I'm using to test sending messages to the server:

public void SendMessage(string msg)
{
    Console.WriteLine("Client sending message:\n" + msg);

    var sendRequest = new RestRequest("/send");
    sendRequest.ContentType = ContentType.TEXT;
    sendRequest.HttpMethod = HttpMethod.POST;
    sendRequest.Encoding = Encoding.UTF8;

    sendRequest.Payload = msg;
    sendRequest.ContentLength = sendRequest.Payload.Length;

    // This try shouldn't really be necessary if I've covered all the WebExceptionHandler cases
    try
    {
        var response = _client.Execute(sendRequest);

        if (response != null)
        {
            Console.WriteLine($"Client received response ({response.StatusCode}):\n" + response.GetContent());
        }
    }
    // This shouldn't really call when a ConnectFailure occurs, but it does as you'll see with my example.
    catch(WebException e)
    {
        bool contains = _client.WebExceptionHandlers.ContainsKey(e.Status);
        Console.WriteLine($"Client {(contains ? "contains" : "doesn't contain")} a handler for this error status ({e.Status})");
    }
}

With that RestClient implementation out of the way, and assuming I have a RestServer implementation that works with a Route setup to handle the POST request (I can share that code with you as well if you need to see it), here is the sample code I'm using to test disconnection handling and other failures:

class Program
{
    static void Main(string[] args)
    {
        Client theClient = new Client("localhost", 8884);

        using (Server theServer = new Server("localhost", 8884))
        {
            theClient.SendMessage("Test");
        }

        theClient.SendMessage("Test 2");

        using (Server theServer = new Server("localhost", 8884))
        {
            theClient.SendMessage("Test 3");
        }

        return;
    }
}

The output of this test is the following:

Client sending message:
Test
Server received message:
Test
Client received response (Ok):
<h1>Ok</h1>
Server is stopping.
Client sending message:
Test 2
Client contains a handler for this error status (ConnectFailure)
Client sending message:
Test 3
Server received message:
Test 3
Client received response (Ok):
<h1>Ok</h1>
Server is stopping.

As you can see, the call to theClient.SendMessage("Test 2"); results in an exception being thrown in the RestClient's Execute method rather than the WebExceptionHandler being invoked. Looking at the source code for Grapevine.RestClient, I am lead to believe that the Client.GenericExceptionHandler should have been called instead of running into an exception.

// Excerpt from RestClient.Execute(IRestClient) method
try
{
    var httpresponse = (HttpWebResponse)request.GetResponse();
    var elapsed = stopwatch.ElapsedMilliseconds;
    response = new RestResponse(httpresponse) { ElapsedTime = elapsed };
}
catch (WebException e)
{
    var elapsed = stopwatch.ElapsedMilliseconds;

    // THIS IS THE ISSUE
    // From my own example, WebExceptionHandlers does indeed contain the key, yet it doesn't execute this conditional code
    if (WebExceptionHandlers.ContainsKey(e.Status))
    {
        WebExceptionHandlers[e.Status].Invoke(this, restRequest, e);
        return null;
    }
    if (e.Response == null) throw;

    var httpresponse = (HttpWebResponse)e.Response;
    response = new RestResponse(httpresponse)
    {
        ElapsedTime = elapsed,
        Error = e.Message,
        ErrorStatus = e.Status
    };
}
mrabey commented 3 years ago

Upon further analysis, it seems that none of the WebExceptionHandlers are being used upon encountering a WebException returned from the HttpWebRequest.GetResponse() call. I shortened the timeout on the request to 100 milliseconds and even though there is an exception handler for the Timeout WebExceptionStatus, it also falls into the try-catch block of my Client.SendMessage method rather than into the Client.GenericExceptionHandler method that is mapped to handle these WebExceptionStatus values.

scottoffen commented 3 years ago

I'm happy to take a look at this (and thanks for the code samples, that will make it easy!), but let me preface my help with a question...

I've removed the Client feature from Grapevine 5 in favor of recommending people use IHttpClientFactory for a number of reasons, but by far the largest being that I didn't think anyone was actually using Grapevine.Client. If I'm mistaken in this, I'll happily look at adding some version of them back (especially since I'm still in prereleases with GV5), but I'll need some community guidance on what would be the most useful implementation. Do you think this would be worth spending some time on?

In the specific issue you mentioned above, using IHttpClientFactory with Polly or some other resilience and transient-fault-handling library would be my preferred approach, but I don't know all the restrictions you are working under, either.

That being said, I'll take a look at this and see what I can come up with. As always, thanks for using Grapevine!

scottoffen commented 3 years ago

@OmnipresentObserver What version of .NET Framework are you using?

I'm trying this now using a console app targeting netcoreapp2.1, and it seems to be working fine. However, my client isn't getting a ConnectFailure error, but an UnknownError. When I register a handler for that, it works just fine...

...except for the server not being able to read the InputStream...

So, I'll look a little deeper, but knowing what you are targeting might help overall.

mrabey commented 3 years ago

@scottoffen,

In this particular case, I am running on .NET Framework 4.7.2 in a console application. I could package up the test application to see if you can reproduce what I'm seeing.

As for your question about the Client usage in Grapevine 5, I'm more partial to leaning on IHttpClientFactory like you are. Sadly, the code I'm dealing with I adopted and it was already using Grapevine.Client and I don't have the time to re-design. So, I will work with what I have and improve it.

scottoffen commented 3 years ago

I could package up the test application to see if you can reproduce what I'm seeing.

Please do, that would help tremendously. Meanwhile, I'll pull down and install 4.7.2 in preparation.

mrabey commented 3 years ago

As mentioned earlier, here is the project that includes the Grapevine test running in .NET Framework 4.7.2.

GrapevineTest.zip

scottoffen commented 3 years ago

Thanks for the code, that made tracking down the issue a piece of cake. The reason it isn't firing the exception handler is because the exception is being thrown on the call to RestRequest.ToHttpWebRequest() which is (currently) outside of the the try/catch block where the exception handler would be executed.

I'll adjust that try/catch block and publish a new version.