Ocelot icon indicating copy to clipboard operation
Ocelot copied to clipboard

HTTP status 499 seems inappropriate when gateway times out waiting on server

Open honestegg opened this issue 2 years ago • 8 comments
trafficstars

Expected Behavior / New Feature

When the gateway (Ocelot) times out waiting for the server to respond, I would expect an HTTP error code of 504, not 499. I wouldn't ever expect a 499 to be returned to the client -- that seems like an error that would just end up in the logs indicating that the client cancelled the request.

Actual Behavior / Motivation for New Feature

With the current behavior, a 499 is returned when Ocelot times out waiting for the server to respond.

Specifications

  • Version: 17.0.0

honestegg avatar Aug 29 '23 14:08 honestegg

Hi Brian! Thanks for your interest in Ocelot!

I laughed a little at your username. 🤣


Well... A lot of people indicated that HTTP statuses are incorrect/wrong/strange. It seems we have an ugly status overriding feature made by Tom:

So, this is Tom's design to override status codes of internal Ocelot's events to remap status to HTTP one. We have special class which is responsible for codes mapping: ErrorsToHttpStatusCodeMapper I have no idea why did you receive 499 for timeout error, but the source code says you should receive 503 status for timeouts. Anyway even 503 Service Unavailable is incorrect too if a timeout happen because it should be 504 Gateway Timeout, as you've explained in the description above.

raman-m avatar Sep 06 '23 09:09 raman-m

Question

Which logs in, on what side did you find the 499 error? Did you catch 499 in the client (Postman, browser's Network tool)? I am interested in the side who generates this error.

It seems we have to debug and investigate a behavior in case of OcelotErrorCode.RequestCanceled events. Could you debug the user case once again please? Any additional info & details on this error are highly desirable. Any uploads of custom solution to GitHub is also welcomed!


In my own feature branch for #1678 I did some improvements of error codes. But this is docs improvement, not technical change. Meanwhile I expect we are able to write change request to fix /enhance your user case with 499 error. Are you going to contribute by real PR or generate ideas for future PRs?


P.S. In my opinion Ocelot should forward original statuses to upstream clients, but I haven't explored HTTP status logic yet.

raman-m avatar Sep 06 '23 10:09 raman-m

FYI Currently it is impossible to cancel request to downstream service because of absent cancellation token for HttpRequest class. But we have ready PR #1367 to fix this cancellation problem. Failure to cancel a request can result in timeouts in some cases. And it seems, Ocelot should cancel request at all after timeout event. So, we could develop this idea...

raman-m avatar Sep 06 '23 10:09 raman-m

@RaynaldM Please, join the discussion!

raman-m avatar Sep 06 '23 10:09 raman-m

FYI Currently it is impossible to cancel request to downstream service because of absent cancellation token for HttpRequest class. But we have ready PR #1367 to fix this cancellation problem. Failure to cancel a request can result in timeouts in some cases. And it seems, Ocelot should cancel request at all after timeout event. So, we could develop this idea...

Yeah, I found this bug while trying to figure out why we were getting a 499. I submitted a PR to fix it (#1688), not realizing a fix had already been submitted 3 years ago, but just hadn't been merged in.

honestegg avatar Sep 06 '23 14:09 honestegg

Question

Which logs in, on what side did you find the 499 error? Did you catch 499 in the client (Postman, browser's Network tool)? I am interested in the side who generates this error.

It seems we have to debug and investigate a behavior in case of OcelotErrorCode.RequestCanceled events. Could you debug the user case once again please? Any additional info & details on this error are highly desirable. Any uploads of custom solution to GitHub is also welcomed!

In my own feature branch for #1678 I did some improvements of error codes. But this is docs improvement, not technical change. Meanwhile I expect we are able to write change request to fix /enhance your user case with 499 error. Are you going to contribute by real PR or generate ideas for future PRs?

P.S. On my opinion Ocelot should forward original statuses to upstream clients, but I haven't explored HTTP status logic yet.

The simplest fix would be in the HttpExeptionToErrorMapper class:

public class HttpExeptionToErrorMapper : IExceptionToErrorMapper
{
    private readonly Dictionary<Type, Func<Exception, Error>> _mappers;

    public HttpExeptionToErrorMapper(IServiceProvider serviceProvider)
    {
        _mappers = serviceProvider.GetService<Dictionary<Type, Func<Exception, Error>>>();
    }

    public Error Map(Exception exception) 
// pass in the HttpContext so the RequestAborted.IsCancellationRequested can be checked
// or just pass in a bool indicating whether the request has been cancelled
    {
        var type = exception.GetType();

        if (_mappers != null && _mappers.ContainsKey(type))
        {
            return _mappers[type](exception);
        }

        if (type == typeof(OperationCanceledException) || type.IsSubclassOf(typeof(OperationCanceledException)))
        {
// check if IsCancellationRequested to return 499... otherwise return 504
            return new RequestCanceledError(exception.Message);
        }

        if (type == typeof(HttpRequestException))
        {
            return new ConnectionToDownstreamServiceError(exception);
        }

        return new UnableToCompleteRequestError(exception);
    }
}

When I get some time I can try to get a PR together.

honestegg avatar Sep 06 '23 14:09 honestegg

@honestegg FYI #1211 fixes the name of the HttpExeptionToErrorMapper class. 😄

raman-m avatar Sep 06 '23 14:09 raman-m

Related to

  • #2072

raman-m avatar May 17 '24 12:05 raman-m