Skip to content

sway-lsp: Fix the double read-lock bug in session #7145

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

Merged
merged 1 commit into from
May 5, 2025

Conversation

Chain-Fox
Copy link
Contributor

@Chain-Fox Chain-Fox commented May 3, 2025

Description

Fix #7144

way-lsp contains a possible double read-lock bug.

The lock is a parking_lot::RwLock:
The first read lock is on L227 in completion_items
The second read lock is on L235 in completion_items

let engines = self.engines.read();
let fn_tokens =
self.token_map
.tokens_at_position(&engines, uri, shifted_position, Some(true));
let fn_token = fn_tokens.first()?.value();
let compiled_program = &*self.compiled_program.read();
if let Some(TypedAstToken::TypedFunctionDeclaration(fn_decl)) = fn_token.as_typed() {
if let Some(program) = &compiled_program.typed {
let engines = self.engines.read();

The solution is to remove the redundant second read lock
After the fix, the possible double read-lock will be eliminated, and the performance may be better.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@Chain-Fox Chain-Fox requested a review from a team as a code owner May 3, 2025 17:06
@fuel-cla-bot
Copy link

fuel-cla-bot bot commented May 3, 2025

Thanks for the contribution! Before we can merge this, we need @Chain-Fox to sign the Fuel Labs Contributor License Agreement.

Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It does seem like this line was unnecessary

@JoshuaBatty JoshuaBatty requested a review from a team May 5, 2025 00:09
Copy link

codspeed-hq bot commented May 5, 2025

CodSpeed Performance Report

Merging #7145 will not alter performance

Comparing Chain-Fox:double-lock-fix (2d036f8) with master (324f04f)

Summary

✅ 22 untouched benchmarks

@JoshuaBatty JoshuaBatty enabled auto-merge (squash) May 5, 2025 03:01
@JoshuaBatty JoshuaBatty merged commit 556b95a into FuelLabs:master May 5, 2025
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sway-lsp: a possible double read-lock in session
3 participants