|
| 1 | +# ADR-004: Explicit timeout required for all lock operations |
| 2 | + |
| 3 | +## Status |
| 4 | + |
| 5 | +Accepted |
| 6 | + |
| 7 | +## Context |
| 8 | + |
| 9 | +When acquiring locks, developers must decide how long to wait for lock availability. There are several approaches: |
| 10 | + |
| 11 | +1. **Optional timeout with default** — `acquire($timeout = null)` where `null` means "wait forever" |
| 12 | +2. **Separate methods** — `tryAcquire()` (non-blocking) vs `acquire()` (blocking forever) vs `acquireWithTimeout($timeout)` |
| 13 | +3. **Explicit timeout everywhere** — All methods require explicit `TimeoutDuration` parameter |
| 14 | + |
| 15 | +The choice affects: |
| 16 | +- API verbosity |
| 17 | +- Risk of accidental infinite blocking |
| 18 | +- Clarity of developer intent |
| 19 | +- Consistency across the library |
| 20 | + |
| 21 | +## Decision |
| 22 | + |
| 23 | +**All lock acquisition methods MUST require an explicit `TimeoutDuration` parameter.** |
| 24 | + |
| 25 | +There is no "default timeout" and no "wait forever" convenience method. |
| 26 | + |
| 27 | +### Rationale |
| 28 | + |
| 29 | +1. **Explicit intent over convenience** |
| 30 | + - Forcing timeout specification makes developers think about lock contention scenarios |
| 31 | + - No accidental infinite waits that hang production systems |
| 32 | + - Clear documentation: "how long is this willing to wait?" |
| 33 | + |
| 34 | +2. **Consistent API surface** |
| 35 | + - `tryAcquire(TimeoutDuration::zero())` — non-blocking |
| 36 | + - `tryAcquire(TimeoutDuration::ofSeconds(5))` — 5-second timeout |
| 37 | + - `tryAcquire(TimeoutDuration::ofMinutes(1))` — 1-minute timeout |
| 38 | + - No special cases, no optional parameters, no nulls |
| 39 | + |
| 40 | +3. **Production safety** |
| 41 | + - Infinite blocking is almost never correct in production |
| 42 | + - If truly needed: `TimeoutDuration::ofHours(24)` makes intent explicit |
| 43 | + - Code review catches unreasonable timeouts |
| 44 | + |
| 45 | +4. **Alignment with PostgreSQL philosophy** |
| 46 | + - PostgreSQL `lock_timeout` setting requires explicit value |
| 47 | + - No "wait forever" is recommended practice |
| 48 | + - Library enforces good practices by design |
| 49 | + |
| 50 | +### Examples |
| 51 | + |
| 52 | +```php |
| 53 | +// ✅ GOOD: Explicit timeout, clear intent |
| 54 | +$lock->tryAcquire(TimeoutDuration::zero()); // immediate |
| 55 | +$lock->tryAcquire(TimeoutDuration::ofSeconds(5)); // 5s max |
| 56 | +$lock->tryAcquire(TimeoutDuration::ofMilliseconds(100)); // 100ms max |
| 57 | + |
| 58 | +// ❌ BAD: Would allow these anti-patterns if timeout was optional |
| 59 | +$lock->tryAcquire(); // unclear: blocking or non-blocking? |
| 60 | +$lock->acquire(); // dangerous: infinite wait |
| 61 | +$lock->acquire(null); // ambiguous: what does null mean? |
| 62 | +``` |
| 63 | + |
| 64 | +## Consequences |
| 65 | + |
| 66 | +### Positive |
| 67 | + |
| 68 | +- **No accidental infinite waits** — Every lock operation has bounded wait time |
| 69 | +- **Self-documenting code** — Timeout value signals expected lock contention |
| 70 | +- **Easier debugging** — No mystery hangs from forgotten timeout configuration |
| 71 | +- **Consistent with library philosophy** — Transaction-level locks preferred (auto-release), session-level as escape hatch with explicit bounds |
| 72 | + |
| 73 | +### Negative |
| 74 | + |
| 75 | +- **Slightly more verbose** — Must write `TimeoutDuration::zero()` instead of just `tryAcquire()` |
| 76 | +- **No "blocking acquire"** — If you need long timeout, must specify large value explicitly |
| 77 | + |
| 78 | +### Mitigation |
| 79 | + |
| 80 | +The verbosity cost is minimal: |
| 81 | +- `TimeoutDuration::zero()` is 22 characters — acceptable for safety gained |
| 82 | +- IDE autocomplete makes it fast to type |
| 83 | +- Named constructors are readable: `ofSeconds(5)` is clearer than `5000` milliseconds |
| 84 | + |
| 85 | +## Alternatives Considered |
| 86 | + |
| 87 | +### Alternative 1: Optional timeout with sensible default |
| 88 | + |
| 89 | +```php |
| 90 | +public function tryAcquire(?TimeoutDuration $timeout = null): bool |
| 91 | +{ |
| 92 | + $timeout ??= TimeoutDuration::ofSeconds(30); // default |
| 93 | + // ... |
| 94 | +} |
| 95 | +``` |
| 96 | + |
| 97 | +**Rejected because:** |
| 98 | +- Hidden default makes code less clear |
| 99 | +- What is "sensible" varies by use case (background job vs user request vs batch process) |
| 100 | +- Default encourages not thinking about timeout strategy |
| 101 | + |
| 102 | +### Alternative 2: Separate methods for blocking vs non-blocking |
| 103 | + |
| 104 | +```php |
| 105 | +public function tryAcquire(): bool; // non-blocking |
| 106 | +public function acquireWithTimeout(TimeoutDuration $t): bool; // explicit timeout |
| 107 | +public function acquire(): void; // blocking forever, throws on failure |
| 108 | +``` |
| 109 | + |
| 110 | +**Rejected because:** |
| 111 | +- Three methods instead of one increases API surface |
| 112 | +- `acquire()` encourages infinite blocking (bad practice) |
| 113 | +- Inconsistent: why does one need timeout but others don't? |
| 114 | + |
| 115 | +### Alternative 3: Global timeout configuration |
| 116 | + |
| 117 | +```php |
| 118 | +$lock = new PostgresSessionLock($connection, $key, defaultTimeout: TimeoutDuration::ofSeconds(30)); |
| 119 | +$lock->tryAcquire(); // uses configured default |
| 120 | +``` |
| 121 | + |
| 122 | +**Rejected because:** |
| 123 | +- Still allows per-call override → two ways to do same thing |
| 124 | +- Default stored in object state → less visible at call site |
| 125 | +- What if different operations need different timeouts? |
| 126 | + |
| 127 | +## Implementation Notes |
| 128 | + |
| 129 | +### For stateless `PostgresAdvisoryLocker` |
| 130 | + |
| 131 | +All public methods require `TimeoutDuration $timeoutDuration` parameter: |
| 132 | + |
| 133 | +```php |
| 134 | +public function acquireTransactionLevelLock( |
| 135 | + ConnectionAdapterInterface $dbConnection, |
| 136 | + PostgresLockKey $key, |
| 137 | + TimeoutDuration $timeoutDuration, // ✅ Required |
| 138 | + PostgresLockAccessModeEnum $accessMode = PostgresLockAccessModeEnum::Exclusive, |
| 139 | +): TransactionLevelLockHandle; |
| 140 | +``` |
| 141 | + |
| 142 | +**No methods like:** |
| 143 | +- ❌ `tryAcquire()` — ambiguous |
| 144 | +- ❌ `acquire()` — dangerous infinite wait |
| 145 | +- ❌ `acquireOrFail()` — when does it fail? |
| 146 | + |
| 147 | +### For `TimeoutDuration` value object |
| 148 | + |
| 149 | +Consider adding named constructors for common cases: |
| 150 | + |
| 151 | +```php |
| 152 | +final class TimeoutDuration |
| 153 | +{ |
| 154 | + public static function zero(): self; |
| 155 | + public static function ofMilliseconds(int $ms): self; |
| 156 | + public static function ofSeconds(int $s): self; |
| 157 | +} |
| 158 | +``` |
| 159 | + |
| 160 | +## References |
| 161 | + |
| 162 | +- PostgreSQL lock_timeout documentation: https://www.postgresql.org/docs/current/runtime-config-client.html#GUC-LOCK-TIMEOUT |
0 commit comments