search

mollie / mollie-api-php

Mollie API client for PHP
http://www.mollie.com
BSD 2-Clause "Simplified" License
552 stars 191 forks source link

v1 API Client timeout is hardcoded to 10 seconds #414

Closed holtkamp closed 5 years ago

holtkamp commented 5 years ago

Specifications

Describe the issue

Currently the CURL timeout of the API Client is hardcoded to 10 seconds:

https://github.com/mollie/mollie-api-php/blob/b385e285d7d0b93807825166beca0e3a11fbcb5c/src/Mollie/API/Client.php#L353

Is this hard coded on purpose? In some cases (when loading a lot of Invoice with a lot of InvoiceLines), this limit is reached causing an error:

[Mollie_API_Exception_ConnectionError]                                                                       
  Unable to communicate with Mollie (28): Operation timed out after 10001 milliseconds with 0 bytes received.

  at /app/vendor/mollie/mollie-api-php/src/Mollie/API/Exception/ConnectionError.php:51
 Mollie_API_Exception_ConnectionError::fromCurlFailure() at /app/vendor/mollie/mollie-api-php/src/Mollie/API/Client.php:411
 Mollie_API_Client->performHttpCall() at /app/vendor/mollie/mollie-api-php/src/Mollie/API/Resource/Base.php:332
 Mollie_API_Resource_Base->performApiCall() at /app/vendor/mollie/mollie-api-php/src/Mollie/API/Resource/Base.php:206
 Mollie_API_Resource_Base->rest_list() at /app/vendor/mollie/mollie-api-php/src/Mollie/API/Resource/Base.php:317
 Mollie_API_Resource_Base->all()

Loading less entries in a more gradual way might be a solution, but bumping the timeout a few seconds might as well 🤓

sandervanhooft commented 5 years ago

Interesting, haven't reached that threshold before.

I'm not a big fan of increasing the threshold here, not a lot of users are likely to experience this problem. And 10 seconds seems like a sane default to me.

You could override the timeout setting by injecting your own Guzzle client.

And/or Mollie could look into performance issues.

@willemstuursma any thoughts here?

holtkamp commented 5 years ago

I'm not a big fan of increasing the threshold here, not a lot of users are likely to experience this problem. And 10 seconds seems like a sane default to me.

I agree, this issue is more of a "why is it hard-coded" question.

Making it configurable with a max at 10 seems a valid approach. Was just wondering about the ideas behind making it hard coded.

You could override the timeout setting by injecting your own Guzzle client.

Note this is about API Client v1, seems not possible to use own client there 😄

sandervanhooft commented 5 years ago

Note this is about API Client v1, seems not possible to use own client there 😄

Ah ok. Is upgrading an option? I don't have much history with v1, but taking a PR for overriding the timeout seems reasonable to me.

holtkamp commented 5 years ago

Yeah, upgrading is on the roadmap.

Although currently it is not a very high prio due to the MVP approach (read: it works and it is supported at least until July 2023, so why bother to upgrade now?

For now limiting the invoices to the last two years (24) does the trick. Just wanted to report this.

sandervanhooft commented 5 years ago

Thanks, closing this for now.

willemstuursma commented 5 years ago

What API call were you doing @holtkamp ? If it is slow we'll fix it.

holtkamp commented 5 years ago

I was listing "all" Invoices using https://docs.mollie.com/reference/v1/invoices-api/list-invoices

The default limit is 250, have now narrowed it down to 24 (last 24 months).

Our first Invoice is from 01-09-2016, so I think "too much info" had to be loaded.

feyyazesat commented 5 years ago

@holtkamp We have deployed a fix related to the list-invoices's page speed. Could you please place your feedback whether the issue you have with it fixed or not?

holtkamp commented 5 years ago

Hi @feyyazesat, thanks for diving into this. Just had a look and the first time a timeout occurs, after that all 39 Invoices (including Lines and Settlements) load in ~3 seconds 👍

So was there a problem in the caching mechanism (since first run still caused a timeout)?

feyyazesat commented 5 years ago

Hi @holtkamp No it wasn't about caching, but there was a costly internal function running so slow we optimised that. There are many db queries to create list of invoices, most probably the first request fails but query results are cached so that loads well on later requests.

holtkamp commented 5 years ago

Good to hear some kind of "cause" was found, thanks all involved for diving into this!

holtkamp commented 4 years ago

@willemstuursma enountered hitting this timeout again last day while attempting to create a payment of check a payment status.


2020-04-01T00:05:34.203+02:00
2020-04-08T11:25:34.008+02:00
2020-04-08T11:32:21.934+02:00
2020-04-08T11:32:42.760+02:00
2020-04-08T11:46:30.813+02:00

Of course this can be due to network congestion, hickups, etc, but what is the recommended approach to deal with it? Report back to the user: "try again later" is a bit dull 😉

My initial attempt would be to increase the timeout...

willemstuursma commented 4 years ago

Hi @holtkamp. We had some minor API slowness.

For creating a payment you can either increase the time out or retry.

For getting the status: just return an HTTP 500 when the timeout is hit and we will try to call the webhook again, later.

holtkamp commented 4 years ago

We had some minor API slowness.

Ok,

For creating a payment you can either increase the time out or retry.

Mmm, in v1 API Client the time-out is still hard-coded to 10 seconds, as that is what this issue is about, so maybe it can be re-opened to keep it on the radar?

Retrying is what we do: display an error suggesting to retry...

https://github.com/mollie/mollie-api-php/issues/414#issuecomment-537970596 aimed at a PR, which I can try to come up with...