Is it concurrency safe?

[jitendra-1217]

laravel-idempotency/src/Idempotency.php

Lines 31 to 37 in c40b126

	if (Cache::has($requestId)) {
	return Cache::get($requestId);
	}
	$response = $next($request);
	$response->header(self::IDEMPOTENCY_HEADER, $requestId);
	Cache::put($requestId, $response, self::EXPIRATION_IN_MINUTES);

^ This does not look concurrency safe. I.e. multiple requests at once with same request id will actually execute it twice.

I wanted to understand if you think that is not a practical case at all and hence intentionally not handled.

[javidalpe]

Hi! Thanks for reviewing the code. You are totally right! If the $response = $next($request); takes 3 secs, for example, any request within 3 sec with the same idempotency key with will execute twice.

Do you have any suggestion to fix this issue? What about:
Given 2 almost concurrent requests A and B with same idempotency key X.

• Request A arrives.
• Request A put a flag 'work in progress' on cache for key X.
• Request A handler begins.
• Request B arrives.
• Request B read 'work in progress' from the cache using key X.
• Request B waits until the cache changes.
• Request A handler ends.
• Request A put the response on the cache for key X and releases.
• Request B read the response from cache and releases.

[jitendra-1217]

The suggested approach basically makes application single threaded i.e. can only serve one request at a time even if we have multiple servers/app instances/threads etc. Which does not seem like issue because it locks on given idempotency key.

But same approach can be extended to reduce the single-threadedness (can't think of other term :) ), e.g. that 'work in progress' lock could be route_name + user_id(or any client identifier). Not needed.

Updated per below comments to avoid mis read.

[javidalpe]

Referring Brandur blog post about idempotency

When performing a request, a client generates a unique ID to identify just that operation and sends it up to the server along with the normal payload.

So yes, it makes the app single threaded for each different idempotency key, which seems ok to me!

[jitendra-1217]

Actually yeah, you're right.
This should work. I must have been thinking something else and mixed up.

[Mdhesari]

@javidalpe

I can fix this with a pr if you are ok with it