Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
honestegg opened this issue Aug 29, 2023 · 8 comments
Assignees
Labels
bug Identified as a potential bug documentation Needs a documentation update proposal Proposal for a new functionality in Ocelot question Initially seen a question could become a new feature or bug or closed ;)

Comments

@honestegg
Copy link

honestegg commented Aug 29, 2023

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
@raman-m raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot documentation Needs a documentation update labels Sep 6, 2023
@raman-m
Copy link
Member

raman-m commented Sep 6, 2023

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
Copy link
Member

raman-m commented Sep 6, 2023

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 raman-m added the question Initially seen a question could become a new feature or bug or closed ;) label Sep 6, 2023
@raman-m
Copy link
Member

raman-m commented Sep 6, 2023

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
Copy link
Member

raman-m commented Sep 6, 2023

@RaynaldM
Please, join the discussion!

@honestegg
Copy link
Author

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
Copy link
Author

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
Copy link
Member

raman-m commented Sep 6, 2023

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

@raman-m
Copy link
Member

raman-m commented May 17, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug documentation Needs a documentation update proposal Proposal for a new functionality in Ocelot question Initially seen a question could become a new feature or bug or closed ;)
Projects
None yet
Development

No branches or pull requests

2 participants