fix(auth): prevent deadlock and unhandled rejections in processLock#2019
fix(auth): prevent deadlock and unhandled rejections in processLock#2019mayur9210 wants to merge 2 commits into
Conversation
- Replace Promise.race timeout pattern with flag-based approach to avoid unhandled promise rejections when lock operation wins the race - Ensure PROCESS_LOCKS promise always resolves so subsequent operations don't deadlock waiting on failed operations - Fix test expectations: operation 0 acquires lock immediately (no wait) so it completes successfully, not times out - Remove unused lockAcquired variable and simplify error handling
There was a problem hiding this comment.
Pull request overview
This pull request fixes critical unhandled promise rejections and potential deadlocks in the processLock function by replacing the Promise.race timeout pattern with a flag-based approach.
Changes:
- Replaced
Promise.racewith a flag-based timeout mechanism that sets atimeoutErrorflag and checks it after acquiring the lock, eliminating unhandled promise rejections - Modified error handling in
PROCESS_LOCKSto always resolve (never reject) so subsequent operations can proceed without deadlocking - Added comprehensive tests for deadlock scenarios and rapid successive operations with mixed timeouts
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/core/auth-js/src/lib/locks.ts | Refactored processLock implementation from Promise.race to flag-based timeout, improved error handling in PROCESS_LOCKS chain, removed some JSDoc documentation |
| packages/core/auth-js/test/lib/locks.test.ts | Added two new tests covering deadlock prevention and rapid successive operations with mixed timeouts |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ing queued operations
|
This PR has been marked as stale because it has not had activity for 90 days. Please rebase and update if this is still needed. |
|
We're working on another solution for a lockless sdk. Thank you very much for your contribution. I am going to close this PR, but keeping it in mind and in our list of potential fixes for our problematic lock mechanism, which will hopefully go away. Please do not be discouraged from contritbuting again in the future! Contributions like yours are what make Supabase what it is today! 💚 |
Summary
Fixes unhandled promise rejections and potential deadlocks in
processLockby replacing thePromise.racetimeout pattern with a flag-based approach.Problem
The
processLockfunction would crash Node.js with unhandled promise rejections when running tests with rapid successive lock operations that have mixed timeouts.The crash occurred because:
processLockusesPromise.race([lockOperation(), timeoutPromise])to implement acquire timeoutlockOperation()wins the race (lock acquired before timeout), the timeout promise still fires laterPROCESS_LOCKS[name]promise chain would re-throw errors, causing subsequent operations waiting on it to receive unhandled rejections or potentially deadlockSolution
Promise.racewith a flag-based approach: set atimeoutErrorflag in the setTimeout callback, check it after acquiring the lockPROCESS_LOCKS[name]always resolves (never throws) so subsequent operations can proceed without deadlockingRelated
Fixes the
ProcessLockAcquireTimeoutError: Acquiring process lock with name "rapid-test" timed outcrash in@supabase/auth-js:test