-
- Notifications
You must be signed in to change notification settings - Fork 704
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! 🚀