Prevent requests timing out

sandervanhooft commented 4 years ago

Specifications

API Version: v2

Describe the issue

Lately there have been some reports about api requests timing out. Mollie's development teams (@willemstuursma @rares-mollie) expect their DNS to be a significant factor.

While the problem is being addressed at DNS / infrastructure level at Mollie, this issue addresses mitigations that can be implemented within the mollie-api-php client.

There are two approaches to this:

Tweaking the Guzzle client options.
Implementing a retry scheme on the Guzzle client.

I am mentioning both here because these approaches are interlinked.

1. Tweaking the Guzzle client options.

@rares-mollie has suggested to set the following on the Guzzle Options:

TIMEOUT: 10 # already there
CONNECT_TIMEOUT: 2 # new
READ_TIMEOUT: 10 # new

By setting the CONNECT_TIMEOUT to 2 we catch connection issues early on, before the call got through to the API. This ensures we are not getting timeout errors on a POST request that somehow still got through. Meaning we do not get false positive timeouts on calls that can mess things up in the API.

It also means that on connect issues, the retry scheme (when implemented) will kick in after 2 seconds instead of 10.

2. Implementing a retry scheme on the Guzzle client.

I'll look into applying a retry scheme to the Guzzle client using the RetryMiddleware that ships with Guzzle. Probably will require a factory and wrapper class.

This will impact MollieApiClient::_construct(), but it can be done in a backwards compatible way.

When should the middleware be applied? (updated)

All requests (GET, POST, PATCH, DELETE) that result in a CONNECT timeout should be retried.
All GET requests that connected ok but result in a timeout anyway should be retried.

When should the middleware NOT be applied? (updated)

If there was no CONNECT timeout, all other requests should not use the retry middleware, especially the POST and PATCH requests.

Testability

the modified Guzzle client, especially the $decider function which determines when the retry middleware should be applied
this needs to be tested with Guzzle 6 and 7 in Travis
this needs to work with PHP 5.6 and up

sandervanhooft commented 4 years ago

@willemstuursma can you explain how #497 relates here, and if it still is considered part of the solution?

sandervanhooft commented 4 years ago

Related: #492 (timeouts reported)

rares-mollie commented 4 years ago

Please expand the applicability of the retry logic to all request methods as long as the timeout happens within the CONNECT phase, the GET requests may be retried after sending the request body but I consider this an optimisation. The benefit of this change is increased reliability within the connect phase.

sandervanhooft commented 4 years ago

@rares-mollie that makes sense to me, let's focus on the CONNECT phase first.

sandervanhooft commented 4 years ago

My text above now reflects this.

mollie / mollie-api-php