-
-
Notifications
You must be signed in to change notification settings - Fork 713
Add solution for Challenge 7 by nzamulov #683
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
Conversation
WalkthroughIntroduces a new Go package Changes
Sequence DiagramssequenceDiagram
participant User
participant Transfer as Transfer(amt, target)
participant Lock1 as Lock Account 1
participant Lock2 as Lock Account 2
participant Validation as Validation
User->>Transfer: Call Transfer
Transfer->>Validation: Check non-nil target, no self-transfer
alt Validation fails
Validation-->>Transfer: Error
Transfer-->>User: Error returned
else Validation passes
Validation-->>Transfer: OK
Transfer->>Lock1: Acquire lock (lower ID)
Transfer->>Lock2: Acquire lock (higher ID)
Lock1-->>Transfer: Acquired
Lock2-->>Transfer: Acquired
Transfer->>Validation: Check amount, balance, limit
alt Balance check fails
Validation-->>Transfer: InsufficientFundsError
else All checks pass
Transfer->>Transfer: Update balances
Transfer-->>Lock2: Release
Transfer-->>Lock1: Release
Transfer-->>User: nil (success)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
| first, second := a, target | ||
| if a.ID > target.ID { | ||
| first, second = target, a | ||
| } | ||
|
|
||
| first.mu.Lock() | ||
| defer first.mu.Unlock() | ||
| second.mu.Lock() | ||
| defer second.mu.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deadlock risk when two accounts share the same ID
Lock ordering swaps only when a.ID > target.ID. If two distinct accounts happen to share the same ID, goroutine A transferring from account1→account2 locks in the sequence (account1, account2) while goroutine B transferring from account2→account1 also locks (account2, account1). Both goroutines then block forever waiting on the other mutex. Because ID uniqueness is not enforced anywhere, this deadlock is reachable at runtime. To make the ordering unambiguous, add a deterministic tie-breaker (e.g., pointer address) when IDs compare equal so every transfer uses the same lock order.
-import "sync"
+import (
+ "sync"
+ "unsafe"
+)
@@
- first, second := a, target
- if a.ID > target.ID {
- first, second = target, a
- }
+ first, second := a, target
+ if a.ID > target.ID ||
+ (a.ID == target.ID && uintptr(unsafe.Pointer(a)) > uintptr(unsafe.Pointer(target))) {
+ first, second = target, a
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In challenge-7/submissions/nzamulov/solution-template.go around lines 138 to
146, the current lock-ordering swaps only when a.ID > target.ID which leaves a
deadlock when two distinct accounts share the same ID; change the ordering logic
to include a deterministic tie-breaker when IDs are equal (for example compare
the account object pointer addresses or another unique stable value) so that for
any pair of accounts you always choose the same first/second before locking;
implement the comparison so if a.ID == target.ID you fall back to comparing
uintptr(unsafe.Pointer(a)) vs uintptr(unsafe.Pointer(target)) (or another stable
unique key) and then lock first then second in that determined order.
Challenge 7 Solution
Submitted by: @nzamulov
Challenge: Challenge 7
Description
This PR contains my solution for Challenge 7.
Changes
challenge-7/submissions/nzamulov/solution-template.goTesting
Thank you for reviewing my submission! 🚀