Skip to content

fix(pnp): resolve deadlock in pnp project#163

Merged
stormslowly merged 2 commits intomainfrom
fix/deadlock_when_resolve_lots_pnp_request
Feb 28, 2026
Merged

fix(pnp): resolve deadlock in pnp project#163
stormslowly merged 2 commits intomainfrom
fix/deadlock_when_resolve_lots_pnp_request

Conversation

@stormslowly
Copy link
Collaborator

@stormslowly stormslowly commented Feb 28, 2026

Problem

Task A: load_pnp(path_A)
  │
  ├─ find_pnp_manifest(path_A)
  │   └─ pnp_manifest_content_cache.entry(key_A)  →  acquire shard X WRITE LOCK
  │      or_insert_with(...)  →  go manifest
  │      entry.downgrade()    →  WRITE LOCK -> READ LOCK
  │      return Ref(holding shard X READ LOCK)
  │
  ├─ resolve_to_unqualified_via_manifest(pnp_manifest, ...)  ← last Ref point 
  │                                                              but Ref live in generator struct
  │
  ├─ inner_resolver.resolve(path_B).await   ← switch to another task ,but shard X READ LOCK still alive!
  │
  └── Task A waiting inner resolve
       │
       └─ inner load_pnp(path_C)
            └─ find_pnp_manifest(path_C)
                └─ pnp_manifest_content_cache.entry(key_C)
                    └─ key_C hashed to shard X!
                       entry() need shard X 的 WRITE LOCK
                       ❌ shard X  READ LOCK holding by Task A
                       → Deadlocked

solution

Drop DashMap ref ASAP.

Copilot AI review requested due to automatic review settings February 28, 2026 09:33
@gemini-code-assist
Copy link

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request resolves a critical deadlock issue within the pnp project's asynchronous resolution process. The problem stemmed from a DashMap read lock being held across await points, leading to a deadlock when a recursive resolution attempt tried to acquire a write lock on the same shard. The solution involves carefully managing the lifetime of the DashMap reference by introducing a block scope, ensuring the read lock is released promptly before any subsequent asynchronous operations, thereby enhancing the stability and reliability of the resolver.

Highlights

  • Deadlock Resolution: Addressed a deadlock scenario that occurred in the pnp project's resolution logic when using DashMap for manifest caching in an asynchronous context.
  • Resource Management: Ensured that the DashMap read lock, held by the pnp_manifest reference, is explicitly dropped before any await calls, preventing the lock from being held across suspension points.
  • Code Structure: Refactored the resolve method in ResolverGeneric to introduce a block scope, which controls the lifetime of the pnp_manifest reference and its associated lock.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • src/lib.rs
    • Introduced a block scope to ensure the DashMap reference (pnp_manifest) is dropped before await calls, releasing the shard read lock.
    • Restructured the pnp manifest resolution logic to prevent deadlocks in asynchronous contexts.
Activity
  • No activity has been recorded on this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a deadlock in the Yarn Plug’n’Play (PnP) resolver path by ensuring DashMap shard locks are not held across .await points, preventing recursive resolution from blocking on the same shard’s write lock.

Changes:

  • Reworks load_pnp to compute PnP unqualified resolution inside a block scope so the DashMap Ref from find_pnp_manifest is dropped before awaiting.
  • Adds detailed inline commentary documenting the deadlock mechanism and why the new scoping is required.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 28, 2026

Merging this PR will not alter performance

✅ 12 untouched benchmarks


Comparing fix/deadlock_when_resolve_lots_pnp_request (51cf494) with main (e68b85b)

Open in CodSpeed

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses a potential deadlock in the pnp resolver logic. The change correctly identifies that a DashMap read lock was being held across an .await point, which could lead to a deadlock if another task attempted to acquire a write lock on the same data. The fix involves restructuring the code to ensure the lock is released before any asynchronous operations are awaited. This is achieved by using a block to limit the scope of the Ref holding the lock. The implementation is clean, idiomatic, and includes excellent comments explaining the reasoning behind the change, which significantly improves maintainability. The fix appears to be correct and robust.

@stormslowly stormslowly merged commit 79439a0 into main Feb 28, 2026
20 checks passed
@stormslowly stormslowly deleted the fix/deadlock_when_resolve_lots_pnp_request branch February 28, 2026 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants