Skip to content
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

LS: Cancellation support #6263

Merged
merged 1 commit into from
Aug 27, 2024
Merged

LS: Cancellation support #6263

merged 1 commit into from
Aug 27, 2024

Conversation

Draggu
Copy link
Collaborator

@Draggu Draggu commented Aug 23, 2024

fix #6131


This change is Reviewable

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Arcticae, @mkaput, and @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, and @mkaput)


crates/cairo-lang-language-server/src/lib.rs line 152 at r1 (raw file):

    let (service, socket) = Backend::build_service(tricks);
    Server::new(stdin, stdout, socket).concurrency_level(100).serve(service).await;

Suggestion:

    Server::new(stdin, stdout, socket).concurrency_level(SOME_CONSTANT_WITH_A_GOOD_NAME).serve(service).await;

@Draggu Draggu linked an issue Aug 26, 2024 that may be closed by this pull request
Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Arcticae and @Draggu)


-- commits line 4 at r2:

Suggestion:

- f774b6c: LS: Cancellation support

  fix #6131

  commit-id:c6b3800a

crates/cairo-lang-language-server/src/lib.rs line 120 at r2 (raw file):

/// [lib]: crate#running-with-customizations
#[non_exhaustive]
#[derive(Default, Clone)]

We should avoid changing Tricks at all costs. The whole purpose of this thing is to be super stable, so that we won't bother downstream users.


crates/cairo-lang-language-server/src/lib.rs line 139 at r2 (raw file):

/// Number of LSP requests that can be processed concurently.
/// Higher number than default 4 because cancellation will skip not needed ones, and latest ones can
/// be processed.

this sentence is hard to parse: what are ones in this context? where does the default come from? what do you mean be needed?


crates/cairo-lang-language-server/src/lib.rs line 331 at r2 (raw file):

            catch_unwind(AssertUnwindSafe(|| f(&db))).map_err(|err| {
                if err.is::<Cancelled>() {
                    info!("LSP worker thread was cancelled");

not sure we need info level for this. this is probably going to be noisy in long-term, and this is not actionable to the user and self-fixing, isn't it?

Suggestion:

debug!("LSP worker thread was cancelled");

crates/cairo-lang-language-server/src/lib.rs line 514 at r2 (raw file):

            .with_db({
                let open_files = open_files.clone();
                let tricks = self.tricks.clone();

why not just construct new_db outside with_db and move it into it? this way you'd avoid needing to clone tricks

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Arcticae and @Draggu)


crates/cairo-lang-language-server/src/lib.rs line 120 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

We should avoid changing Tricks at all costs. The whole purpose of this thing is to be super stable, so that we won't bother downstream users.

discussed this offline. let's add this Clone due to all the issues lack of it brings in this PR

Copy link
Collaborator Author

@Draggu Draggu left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @Arcticae, @mkaput, and @orizi)


-- commits line 4 at r2:
Done.


crates/cairo-lang-language-server/src/lib.rs line 152 at r1 (raw file):

    let (service, socket) = Backend::build_service(tricks);
    Server::new(stdin, stdout, socket).concurrency_level(100).serve(service).await;

Done.


crates/cairo-lang-language-server/src/lib.rs line 139 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

this sentence is hard to parse: what are ones in this context? where does the default come from? what do you mean be needed?

Done.


crates/cairo-lang-language-server/src/lib.rs line 331 at r2 (raw file):

Previously, mkaput (Marek Kaput) wrote…

not sure we need info level for this. this is probably going to be noisy in long-term, and this is not actionable to the user and self-fixing, isn't it?

Done.

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Arcticae and @orizi)

Copy link
Collaborator

@Arcticae Arcticae left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

fix #6131

commit-id:c6b3800a
Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @orizi)

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Draggu)

Copy link
Member

@mkaput mkaput left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Draggu)

Copy link
Collaborator

@piotmag769 piotmag769 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Draggu)

@Draggu Draggu added this pull request to the merge queue Aug 27, 2024
Merged via the queue into main with commit 2c70d5d Aug 27, 2024
44 checks passed
@orizi orizi deleted the spr/main/c6b3800a branch August 28, 2024 05:57
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.

Cancel pending requests when didChange & et all appear.
5 participants