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

ShouldBeUnique with DynamoDB provider doesn't work as expected with default configuration #43280

Closed
wowu opened this issue Jul 19, 2022 · 2 comments
Assignees
Labels

Comments

@wowu
Copy link

wowu commented Jul 19, 2022

  • Laravel Version: v9.19.0
  • PHP Version: 8.1.6

Description:

According to documentation, implementing ShouldBeUnique on job class is enough to ensure only one instance of given job is present in the queue. However, using dynamodb cache driver without setting $uniqueFor explicitly makes the lock item expire after 1 second, because expires_at is set to current timestamp when $uniqueFor is 0 (default value).

This is the change event in DynamoDB table after putItem request when trying to acquire job lock:

{
  "awsRegion": "eu-central-1",
  "eventID": "4cb2d5b3-4954-451e-8332-01671dd2bf3e",
  "eventName": "INSERT",
  "userIdentity": null,
  "recordFormat": "application/json",
  "tableName": "vapor_cache",
  "dynamodb": {
    "ApproximateCreationDateTime": 1658222276198,
    "Keys": {
      "key": {
        "S": "cloud-api-production:cloud-api-production:laravel_unique_job:App\\Jobs\\<job-name>"
      }
    },
    "NewImage": {
      "value": {
        "S": "s:16:\"D7X1915ojxbQW6sU\";"
      },
      "key": {
        "S": "cloud-api-production:cloud-api-production:laravel_unique_job:App\\Jobs\\<job-name>"
      },
      "expires_at": {
        "N": "1658222276"
      }
    },
    "SizeBytes": 237
  },
  "eventSource": "aws:dynamodb"
}

expires_at is equal to current timestamp (see ApproximateCreationDateTime), so trying to acquire the lock in the following second will work because of used condition:

'ConditionExpression' => 'attribute_not_exists(#key) OR #expires_at < :now',

It seems like the behavior is not consistent across other cache providers, e.g. database cache provider uses lock expiration time of 1 day if no time was provided:

protected function expiresAt()
{
return $this->seconds > 0 ? time() + $this->seconds : Carbon::now()->addDays(1)->getTimestamp();
}

redis provider creates non-expiring item:

public function acquire()
{
if ($this->seconds > 0) {
return $this->redis->set($this->name, $this->owner, 'EX', $this->seconds, 'NX') == true;
} else {
return $this->redis->setnx($this->name, $this->owner) === 1;
}
}

memcached items doesn't expire with expiration time set to 0.

While dynamodb items expire in the next second after the lock is acquired.

Possible solution would be to use 1 day as default expiration value, as in database cache provider case.

Steps To Reproduce:

  1. Use dynamodb cache provider
  2. Create a job implementing ShouldBeUnique that takes longer than a few seconds to complete
  3. Try to dispatch the job two times, more than 1 second apart
  4. The job will be executed concurrently despite implementing ShouldBeUnique

Repository demonstrating the issue: https://github.com/Wowu/laravel-dynamodb-bug-report

@taylorotwell
Copy link
Member

Can you make a PR to fix the issue?

@driesvints
Copy link
Member

We merged a fix. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants