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

Need HTTP request retry to be configurable #2425

Open
crankydillo opened this issue Aug 9, 2022 · 9 comments
Open

Need HTTP request retry to be configurable #2425

crankydillo opened this issue Aug 9, 2022 · 9 comments
Labels
help wanted We need contributions on this type/enhancement A general enhancement

Comments

@crankydillo
Copy link

crankydillo commented Aug 9, 2022

Due to our infrastructure, we need to retry requests, primarily connection establishment related like connection failed/timed out, SSL handshake timeout, etc.. To date, we have tried doing this by adding retry to the following:

  1. The Mono we return from the function given to HttpClient#mapConnect.
  2. The Mono returned from the ConnectionProvider#acquire (via wrapping)

The first option is more promising (thanks @violetagg !), but it still comes with some negatives. We're thinking the second will not work for all the cases we want to retry.

At this time, we're thinking the best approach is to make the retry more configurable. Something like Apache HTTP client retry interface, but not quite that opened ended. I totally get that this is not a slam-dunk on whether or not this should be added given reactor-core's built-in retry concept, but the biggest issue we have using the mapConnect approach above is making sure we don't retry if any application data has been sent across the wire (e.g. request line, headers). The only thing we currently have to solve that problem is to register a ConnectionObserver, but this seems a little too coupled with reactor-http implementation.

This was discussed in gitter here.

The bulk of the retry code I refer to for mapConnect is:

final AtomicBoolean headersSent = new AtomicBoolean(false);

final Retry retryPolicy = Retry.from(companion -> companion.map(sig -> {
    if (!headersSent.get() && sig.totalRetries() < config.getMaxRequestRetry()) {
        return sig.totalRetries();
    }
    throw Exceptions.propagate(sig.failure());
}));

return httpClient
    .observe((connection, newState) -> {
        if (newState == ConnectionObserver.State.CONNECTED) {
            headersSent.set(true);
        }
    }).mapConnect(conn -> {
        return conn.retryWhen(retryPolicy);
    });

Motivation

We have many cases where infrastructure problems can be 'worked around' by doing an immediate request retry. These exceptional cases are broader than what reactor-http currently retries.

Desired solution

Ability to customize retry behavior to suit our infrastructure. Basically, to mimic what we were doing with Apache HTTP client:)

Considered alternatives

Detailed above.

@crankydillo crankydillo added status/need-triage A new issue that still need to be evaluated as a whole type/enhancement A general enhancement labels Aug 9, 2022
@pderop pderop self-assigned this Aug 10, 2022
@pderop pderop added status/need-investigation This needs more in-depth investigation and removed status/need-triage A new issue that still need to be evaluated as a whole labels Aug 10, 2022
@crankydillo
Copy link
Author

@dnagarajan89 pointed out the silliness of my snippet above. It would block retry on any further request that ultimately grabbed a new connection from the pool. While we may can work around that, it does make this feature request more appealing to us:)

@pderop
Copy link
Contributor

pderop commented Aug 10, 2022

Hi @crankydillo ,

I just have read the discussion from Gitter you have mentioned, I'm sorry but I'm still not sure why you can't just leverage the Reactor Retry operator with the mapConnect, as Violeta suggested ?

Now, using the "observe" method in order to track ConnectionObserver.State.CONNECTED events will only work for newly connected connections, but it won't work for connections reused from pool if I'm correct ?

So, why not using the kind of example you provided (without the observe), or some kind of the following example that is filtering specific exceptions for which the failed connection establishment can be retried ? if we get a ConnectException, then the request has not yet been sent, so no risk to retry an already sent request (am I missing something ?):


Retry retryPolicy = Retry.backoff(3, Duration.ofSeconds(2))
    .filter(err -> err instanceof ConnectException || ...)
    .onRetryExhaustedThrow(((retryBackoffSpec, retrySignal) -> retrySignal.failure()));
    
client
    .mapConnect(conn -> conn.retryWhen(retryPolicy))
    .request(HttpMethod.GET)
    .uri("/foo")
    ...

Please indicate the use cases which you can't achieve when filtering the Exception using the kind of above example ?
thanks.

@pderop pderop added for/user-attention This issue needs user attention (feedback, rework, etc...) and removed status/need-investigation This needs more in-depth investigation labels Aug 10, 2022
@crankydillo
Copy link
Author

crankydillo commented Aug 10, 2022

Hi @pderop,

Actually, our first attempt did involve using mapConnect and a list of retry-able exceptions. All our tests passed; however, our product folks asked for behavior we had in an older version of our software, which was to retry unless a certain set of exceptions are thrown and no HTTP-level data has been sent over the wire.

We are discussing bring back our list of retry-able exceptions + mapConnect, but even with that we are dependent on the exception class guaranteeing no data is sent. Furthermore, we may open this configuration up to our users, where it would be nice to have even more guarantees they couldn't possibly trigger request data re-delivery.

I may get in trouble for saying this, but I understand not wanting to implement this unless multiple customers ask for it. On that topic, we are willing to work on this. One of the reasons for this was to see if you had changed your minds wrt even considering this feature or to see if there's a work-around with existing APIs to have configurable retry behavior with a guarantee that no HTTP-level data could be sent across the wire.

@pderop pderop removed the for/user-attention This issue needs user attention (feedback, rework, etc...) label Aug 10, 2022
crankydillo pushed a commit to crankydillo/reactor-netty that referenced this issue Aug 10, 2022
The main issue we can't use generic reactor-core retry concept is due to
the inability to detect if HTTP-level data has been sent across the
wire.

 reactor#2425
crankydillo pushed a commit to crankydillo/reactor-netty that referenced this issue Aug 10, 2022
@violetagg
Copy link
Member

@crankydillo Can you rework the POC in a way so that it accepts Retry configuration?

@violetagg violetagg added the help wanted We need contributions on this label Aug 18, 2022
@violetagg violetagg added this to the General Backlog milestone Aug 18, 2022
@crankydillo
Copy link
Author

I don't see why not @violetagg . Did you happen to look at some of the other parts of the PoC? Totally understand if you're waiting for an official PR.

@violetagg
Copy link
Member

@crankydillo Basically you separated the retry functionality and redirect. I would imaging an API that receives a Retry configuration and we can use it instead of some default Retry configuration. Why I'm asking for a Retry configuration - you need only Exceptions and number of retries but in the future someone may need other configurations.

@crankydillo
Copy link
Author

Totally makes sense. Not sure why I didn't do that to start with. I will try to get this done within the next week or so.

crankydillo pushed a commit to crankydillo/reactor-netty that referenced this issue Aug 27, 2022
The main issue we can't use generic reactor-core retry concept is due to
the inability to detect if HTTP-level data has been sent across the
wire.

 reactor#2425
crankydillo pushed a commit to crankydillo/reactor-netty that referenced this issue Aug 27, 2022
crankydillo added a commit to crankydillo/reactor-netty that referenced this issue Aug 27, 2022
@crankydillo
Copy link
Author

I'm not sure when I'll be able to get back to this. At this time, my team is not asking for it in the near-term. I'm fine closing this if you don't feel it's a feature you need or want to track.

@violetagg
Copy link
Member

I'm not sure when I'll be able to get back to this. At this time, my team is not asking for it in the near-term. I'm fine closing this if you don't feel it's a feature you need or want to track.

Let's keep it for the moment if somebody wants to work on it ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted We need contributions on this type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants