Closed Pimm closed 1 year ago
We don't currently implement Retry-After
indeed. I haven't heard anyone ask for this before, but it's an interesting idea.
One of our test merchants is asking whether we can tag this commit so they can already try it out, e.g. if we tag it as prerelease
they can install it via @mollie/api-client@prerelease
. Is this possible @Pimm?
@Pimm I'm getting the feedback from our test merchant that they would like to configure their own idempotency key as well. Can you make this happen?
This way, they can tie the key to their own back-end. For example if the whole container dies during the request, there is no way of retrying the request if the library only generates its own keys. They would rather generate their own key and provide this to the library explicitly, so they can protect themselves against scenarios that are outside the control of the Mollie library.
I just had a call with @mollierobbert and @fjbender. Thanks for making some time for me on this blackest of Fridays.
The idea @janpaepke and I had which motivated this PR, is that if the Mollie API has a brief hiccup, extra attempts may cause a request to succeed anyway. The feature is designed to be invisible to users of this library. It all happens internally.
The feedback we received, is that integrators would like to have control over the idempotency keys. The current implementation helps in the scenario in which the Mollie API hiccups. However, there are other scenarios in which idempotency keys could help. For instance, if the plug is pulled right as a server does mollieClient.payments.create(…)
, the server could theoretically ‒ once it boots back up ‒ notice the payment creation is still in a queue and then retry with the same key. But for this to be an option, users of this library have to be able to provide their own keys (since there's no way for the server to know which random key was generated last time).
The PHP client also uses idempotency keys since recently, and gives control over the keys.
The PR in the PHP client adds a few methods, most notably:
$mollieApiClient->setIdempotencyKey(…)
and
$mollieApiClient->setIdempotencyKeyGenerator(…)
setIdempotencyKey
sets the key which will be used during the next request (whatever POST
, DELETE
, or PATCH
(?) request that may be). This would not be a good interface for the Node.js client. In JavaScript, setting "global" state like this is frowned upon, not in the least because asynchronous code is common and thus a race condition may cause the key you set with setIdempotencyKey
to be snatched by another request before you do the mollieClient.payments.create(…)
.
We could add an optional idempotencyKey
property in the endpoints which result in a POST
or DELETE
call. One could then do:
mollieClient.payments.create({
…,
idempotencyKey: 'bf4a894b'
})
This is conceptually similar to $mollieApiClient->setIdempotencyKey
in the PHP client, but safe.
We also could allow passing an idempotency key generator when creating the client, like so:
const mollieClient = createMollieClient({
apiKey: 'test_dHar4XY7LxsDOtmnkVtjNVWXLSlXsM',
idempotencyKeyGenerator: (type, data) => {
return `${type}-${data.metadata.orderID}`
}
});
This is conceptually similar to $mollieApiClient->setIdempotencyKeyGenerator
in the PHP client. I do feel this function would have to be provided the data of the thing being created or deleted, as the idempotency key will likely be based on said data. If the function is not provided anything, all it could possibly do is generate a random idempotency key, which is the default behaviour anyway.
Finally, we could give the error thrown by the endpoints which result in a POST
or DELETE
call an idempotencyKey
property. One could then do:
try {
await mollieClient.payments.create(…);
} catch (error) {
const { idempotencyKey } = error;
// Schedule a retry 10 minutes from now with the same key.
}
In the current implementation, we don't retry on network issues. If the network is down, we expect it to be still down five seconds from now. And we don't want to continue retrying for more than a few seconds, because a customer may be gazing at a spinner while the payment for their order is being created. However, there are processes which happen "in the background" by nature, such as performing a recurring payment. We could consider making more attempts and also retrying on network issues in those cases.
I think all these ideas are sensible and since they're not mutually exclusive I would suggest to implement them all.
Retrying after certain timeouts seems like a good idea but would be outside the scope of this PR.
Just to clarify: we would still keep the "invisible" feature of supplying our own idempotency key, right? We'd just allow for it to be overwritten either by supplying one or providing a generation callback.
This also means if a consumer both sets up custom key generation and provides a key with the call the latter "wins".
This does not seem counterintuitive but the hierarchy needs to be documented.
The more I think about idea 2 ‒ the idempotencyKeyGenerator
‒ the less I like it.
Here's why:
function createPayment(data) {
return mollieClient.payments.create({
...data,
idempotencyKey: hash(data.metadata.orderID)
});
}
After discussing this more I tend to agree with this assessment.
It's not that we can't think of any theoretical cases where defining a key generation function might be useful. It's that these cases are very rare and there are valid work-arounds available (see @Pimm's example).
Usually I would say "well if it doesn't hurt, let's just add it". However with this SDK we also have to consider the long term effects of exposing new API. This would have to be supported potentially indefinitely, because one engineer is using it, who could have achieved the same with slightly different code, had he been forced to.
Additionally not adding the generator function makes for a simpler API and less confusion regarding my point above.
With those points in mind, I would suggest to leave out idea 2 and only implement 1 and 3.
So if I understand correctly, you want to move forward with:
All of these make sense to me. 👍
After finishing Idea 3, I noticed that the ApiError
wasn't exposed from the lib.
Additionally there seems to have been an outdated interface, which was never (externally) exposed, either.
Exporting the error allows for type-safe checking like this:
try {...} catch (error) {
if (error instanceof MollieApiError) {
console.log(error.statusCode /* <- is a known property */);
}
}
Consequently I included exposure of the ApiError
as MollieApiError
in the PR: https://github.com/mollie/mollie-api-node/pull/298/commits/aa8f4924b013a67be6bc8a0a6bff6a7f27a6a35c
How do you guys feel about that?
I noticed that the
ApiError
wasn't exposed from the lib.
Someone else actually noticed that just a tad before you did. But that's good! It just confirms we're on the right track.
Additionally there seems to have been an outdated interface, which was never (externally) exposed, either.
Good catch!
Resolves #257.
Concept
Every requests to which the Mollie API responds with a 5×× status code will be re-attempted until:
The idea is that if the Mollie API has a brief hiccup, the extra attempts may cause the request to succeed anyway.
For
POST
andDELETE
requests, an idempotency key is added (in the form of anIdempotency-Key
header). This ensures the Mollie API can distinguish a single request being re-attempted from two separate similarly-looking requests. In effect, this allows this client to safely re-attempt requests.We agreed on an attempt limit of 3 and a (fallback) retry delay of 2 seconds.
We've decided not to retry on network issues, such as no network being available or the request timing out. This client is designed to be run on servers. Unlike on mobile phones with unreliable wireless connections, we expect the network to either work or not work. We don't expect a retry to resolve such network issues.
Limitations
Because it was really easy to implement, we added support for the
Retry-After
header. I haven't seen the Mollie API actually utilise this header, but it can use it to instruct the client to wait longer or shorter than two seconds.But… because we can't imagine it ever being useful, we left out support for the HTTP date form of
Retry-After
. If support is ever needed, there's a library out there to help out.Note
If we do ever decide to retry on timeouts, we should introduce a per-attempt timeout separate from the global timeout which currently exists. If a developer specifies a
timeout
in their call tocreateMollieClient
, retrying after that timeout would be unexpected.