|
| 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( |
| 91 | + ?TimeoutDuration $timeout = null, |
| 92 | + ): bool { |
| 93 | + $timeout ??= TimeoutDuration::ofSeconds(30); // default |
| 94 | + // ... |
| 95 | +} |
| 96 | +``` |
| 97 | + |
| 98 | +**Rejected because:** |
| 99 | +- Hidden default makes code less clear |
| 100 | +- What is "sensible" varies by use case (background job vs user request vs batch process) |
| 101 | +- Default encourages not thinking about timeout strategy |
| 102 | + |
| 103 | +### Alternative 2: Separate methods for blocking vs non-blocking |
| 104 | + |
| 105 | +```php |
| 106 | +public function tryAcquire(): bool; // non-blocking |
| 107 | +public function acquireWithTimeout(TimeoutDuration $t): bool; // explicit timeout |
| 108 | +public function acquire(): void; // blocking forever, throws on failure |
| 109 | +``` |
| 110 | + |
| 111 | +**Rejected because:** |
| 112 | +- Three methods instead of one increases API surface |
| 113 | +- `acquire()` encourages infinite blocking (bad practice) |
| 114 | +- Inconsistent: why does one need timeout but others don't? |
| 115 | + |
| 116 | +### Alternative 3: Global timeout configuration |
| 117 | + |
| 118 | +```php |
| 119 | +$lock = new PostgresSessionLock( |
| 120 | + $connection, |
| 121 | + $key, |
| 122 | + defaultTimeout: TimeoutDuration::ofSeconds(30), |
| 123 | +); |
| 124 | +$lock->tryAcquire(); // uses configured default |
| 125 | +``` |
| 126 | + |
| 127 | +**Rejected because:** |
| 128 | +- Still allows per-call override → two ways to do same thing |
| 129 | +- Default stored in object state → less visible at call site |
| 130 | +- What if different operations need different timeouts? |
| 131 | + |
| 132 | +## Implementation Notes |
| 133 | + |
| 134 | +### For stateless `PostgresAdvisoryLocker` |
| 135 | + |
| 136 | +All public methods require `TimeoutDuration $timeoutDuration` parameter: |
| 137 | + |
| 138 | +```php |
| 139 | +public function acquireTransactionLevelLock( |
| 140 | + ConnectionAdapterInterface $dbConnection, |
| 141 | + PostgresLockKey $key, |
| 142 | + TimeoutDuration $timeoutDuration, // ✅ Required |
| 143 | + PostgresLockAccessModeEnum $accessMode = PostgresLockAccessModeEnum::Exclusive, |
| 144 | +): TransactionLevelLockHandle; |
| 145 | +``` |
| 146 | + |
| 147 | +**No methods like:** |
| 148 | +- ❌ `tryAcquire()` — ambiguous |
| 149 | +- ❌ `acquire()` — dangerous infinite wait |
| 150 | +- ❌ `acquireOrFail()` — when does it fail? |
| 151 | + |
| 152 | +### For `TimeoutDuration` value object |
| 153 | + |
| 154 | +Consider adding named constructors for common cases: |
| 155 | + |
| 156 | +```php |
| 157 | +final class TimeoutDuration |
| 158 | +{ |
| 159 | + public static function zero(): self; |
| 160 | + public static function ofMilliseconds(int $ms): self; |
| 161 | + public static function ofSeconds(int $s): self; |
| 162 | +} |
| 163 | +``` |
| 164 | + |
| 165 | +## References |
| 166 | + |
| 167 | +- PostgreSQL lock_timeout documentation: https://www.postgresql.org/docs/current/runtime-config-client.html#GUC-LOCK-TIMEOUT |
0 commit comments