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

[Lock] Update factory contract #44680

Closed
ro0NL opened this issue Dec 16, 2021 · 1 comment · Fixed by #44706
Closed

[Lock] Update factory contract #44680

ro0NL opened this issue Dec 16, 2021 · 1 comment · Fixed by #44706
Labels

Comments

@ro0NL
Copy link
Contributor

ro0NL commented Dec 16, 2021

Description

LockFactory returns SharedLocksInterface's

public function createLock(string $resource, ?float $ttl = 300.0, bool $autoRelease = true): LockInterface
{
return $this->createLockFromKey(new Key($resource), $ttl, $autoRelease);
}

public function createLockFromKey(Key $key, ?float $ttl = 300.0, bool $autoRelease = true): LockInterface
{
$lock = new Lock($key, $this->store, $ttl, $autoRelease);
$lock->setLogger($this->logger);
return $lock;
}

final class Lock implements SharedLockInterface, LoggerAwareInterface

Should we update the return types?

Also acquireRead() may forward to acquire()

public function acquireRead(bool $blocking = false): bool
{
$this->key->resetLifetime();
try {
if (!$this->store instanceof SharedLockStoreInterface) {
$this->logger->debug('Store does not support ReadLocks, fallback to WriteLock.', ['resource' => $this->key]);
return $this->acquire($blocking);
}

which raises questions about this contract in the first place ... :) perhaps an @method on LockInterface is possible still?

Example

No response

@carsonbot carsonbot added the Lock label Dec 16, 2021
@ro0NL
Copy link
Contributor Author

ro0NL commented Dec 17, 2021

Addiotionally having NoLock implements SharedLockInterface, provided if $factory->test === true would be nice, for testing purposes

Never mind, i'll use InMemoryStore 🤷

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

Successfully merging a pull request may close this issue.

2 participants