ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.28k stars 1.63k forks source link

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

Open honestegg opened 11 months ago

honestegg commented 11 months ago

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

raman-m commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

@RaynaldM Please, join the discussion!

honestegg commented 11 months ago

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 commented 11 months ago

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.

raman-m commented 11 months ago

@honestegg FYI

1211 fixes the name of the HttpExeptionToErrorMapper class. 😄

raman-m commented 3 months ago

Related to