Reloading file in LS after desynchronization. #6752

farmaazon · 2023-05-18T09:43:39Z

Pull Request Description

This PR changes behavior when text/applyChange returned error.

Before we always assumed that the change was not applied, and tried to send full synchronization still assuming old file content. But this was not the case on some errors (timeouts for example). Now we instead reopen the file (getting its actual content) and then make a full invalidation.

Also added a shortcut allowing manual file reloading, what may be useful in some kinds of error (and also allowed me testing of reopening file in the application).

Important Notes

The unit tests of sending text updates were improved: now we actually check if all expected messages are emitted from the IDE.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
~~Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.~~
All code follows the
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed, the GUI was tested when built using ./run ide build.

farmaazon · 2023-05-18T09:44:24Z

Turned to draft after realizing some FIXMEs were left in code.

vitvakatu

Only minor nit-picks.

app/gui/src/model/module/synchronized.rs

vitvakatu · 2023-05-19T09:05:45Z

app/gui/src/model/module/synchronized.rs

+impl Default for LanguageServerContent {
+    fn default() -> Self {
+        Self::Unknown
    }
 }


nit: it's possible to derive Default and mark the needed variant with the #[default] attribute.

Indeed. For some reason, I thought it's possible only when all variants have no fields.

vitvakatu · 2023-05-19T09:15:25Z

app/gui/src/model/module/synchronized.rs

    /// Send to LanguageServer update about received notification about module. Returns the new
    /// content summery of Language Server state.


The second sentence is no longer accurate.

app/gui/src/model/module/synchronized.rs

vitvakatu · 2023-05-19T09:16:51Z

app/gui/src/model/module/synchronized.rs

        new_file: &SourceFile,
        edits: Vec<TextEdit>,
        execute: bool,
-    ) -> impl Future<Output = FallibleResult<ParsedContentSummary>> + 'static {
+    ) -> impl Future<Output = ()> + 'static {
+        println!("---\n{}---\n", new_file.content);


Debug left over?

Removed all printlns.

vitvakatu · 2023-05-19T09:18:53Z

app/gui/src/model/module/synchronized.rs

-                debug!("Actual digest:   {actual_old} => {actual_new}");
-                debug!("Declared digest: {} => {}", edits.old_version, edits.new_version);
-                debug!("New content:\n===\n{new_content}\n===");
+                println!("Actual digest:   {actual_old} => {actual_new}");
+                println!("Declared digest: {} => {}", edits.old_version, edits.new_version);
+                println!("New content:\n===\n{new_content}\n===");


nit: I believe debug! (and other) messages are still visible in regular tests, so this change should be unnecessary.

Co-authored-by: Ilya Bogdanov <fumlead@gmail.com>

Frizi · 2023-05-19T13:25:10Z

app/gui/src/controller/upload.rs

@@ -77,6 +77,8 @@ pub struct FileToUpload<DataProvider> {
 #[derive(Clone, Debug)]
 pub struct FileUploadProcess<DataProvider> {
    bin_connection:  Rc<binary::Connection>,
+    // See FIXME in upload_chunk method.


I don't see any FIXME in upload_chunk method. Is this still necessary?

It's a change committed to a wrong branch.

#6689 also contains the change, once it will be merged, here it should disappear from diff.

Frizi · 2023-05-19T18:03:37Z

QA report 🥦

No issues spotted. I tried opening large projects and doing many edits in quick succession, enabling CPU throttling and even toggling offline mode. The IDE was able to recover from all of those.

…fter_desynchronization

hubertp

Tested on my pet project. I can no longer reproduce Invalid Version errors, so I think it's a step forward. Ideally, it would be cool to somehow notify the user about the reload because I guess they can be surprised about lost of last edit?

wdanilo · 2023-05-23T00:20:52Z

app/gui/src/model/module/plain.rs

+    fn reopen_file_in_language_server(&self) -> BoxFuture<FallibleResult> {
+        info!("Ignoring request for reopening file in the Language Server, because it's not connected");
+        future::ready_boxed(Ok(()))
+    }
 }


wdanilo · 2023-05-23T00:21:27Z

app/gui/src/model/module/synchronized.rs

+#[derive(Clone, Debug, Default)]
 enum LanguageServerContent {
    /// The content is synchronized with our module state after last fully handled notification.
    Synchronized(ParsedContentSummary),
-    /// The content is not synchronized with our module state after last fully handled
-    /// notification, probably due to connection error when sending update.
-    Desynchronized(ContentSummary),
-}
-
-impl LanguageServerContent {
-    fn summary(&self) -> &ContentSummary {
-        match self {
-            LanguageServerContent::Synchronized(content) => &content.summary,
-            LanguageServerContent::Desynchronized(content) => content,
-        }
-    }
+    /// The content is not known due to an unrecognized error received from the Language Server
+    /// after applying the last update. We don't know if, and to what extent it was applied.
+    #[default]
+    Unknown,


Why is it better than Option<ParsedContentSummary>?

wdanilo · 2023-05-23T00:22:05Z

app/gui/src/model/module/synchronized.rs

-        self.language_server.client.close_text_file(file_path).await?;
+        info!("Reopening file {file_path}.");
+        if let Err(error) = self.language_server.client.close_text_file(file_path).await {
+            error!("Error while reopening file {file_path}: Closing operation failed: {error} Trying to open the file anyway.");


wdanilo · 2023-05-23T00:22:36Z

app/gui/src/model/module/synchronized.rs

+    /// Listen for all module changes and sends proper updates to the Language Server.
+    ///


sends -> send

wdanilo · 2023-05-23T00:22:47Z

app/gui/src/model/module/synchronized.rs

-    ) -> impl Future<Output = ()> {
+    /// Listen for all module changes and sends proper updates to the Language Server.
+    ///
+    /// This function returns once the  notifications from underlying [module model](plain::Module)


double space after the

wdanilo · 2023-05-23T00:23:43Z

app/gui/src/model/module/synchronized.rs

        new_file: SourceFile,
-    ) -> impl Future<Output = FallibleResult<ParsedContentSummary>> + 'static {
+    ) -> impl Future<Output = ()> + 'static {


Does the Output not default to () ? If not, this is an IMO surprising design of this lib.

Associated type defaults are not in stable Rust yet and unstable feature is incomplete, especially around trait objects.
rust-lang/rust#29661 ...since 2015!

wdanilo · 2023-05-23T00:25:14Z

app/gui/src/model/module/synchronized.rs

+                    debug!("Updating the LS content digest to: {:?}", summary);
+                    ls_content.replace(LanguageServerContent::Synchronized(summary));
+                }
+                Err(err) => {
+                    error!("Error during sending text change to Language Server: {err}");


Minor inconsistency - some errors use LS, some use Language Server. I think that abbreviations are always worse than full names, so I'd be thankful for moving them to full names instead.

* develop: Use state sent with `GET /directories` instead of querying state separately (#6794) Run TypeScript typechecking and eslint on Lint CI (#6603) Disable cloud backend for unverified users; use local backend as default backend; minor bugfixes (#6779) Reloading file in LS after desynchronization. (#6752) Missing conversion of hash key in EqualsNode (#6803) feat: set constructor args tag values (#6801)

farmaazon added 3 commits May 16, 2023 17:00

Implement the thing, but untested

5a95c53

Improve tests

30b6a46

Add shortcut for reopening

10e7ae6

farmaazon self-assigned this May 18, 2023

farmaazon requested review from MichaelMauderer, wdanilo, kazcw and mwu-tow as code owners May 18, 2023 09:43

farmaazon marked this pull request as draft May 18, 2023 09:44

Linter & docs

cb5c68d

farmaazon marked this pull request as ready for review May 18, 2023 10:17

Merge branch 'develop' into wip/ao/reloading_after_desynchronization

1df90f6

vitvakatu approved these changes May 19, 2023

View reviewed changes

farmaazon and others added 2 commits May 19, 2023 11:22

Update app/gui/src/model/module/synchronized.rs

8b4b522

Co-authored-by: Ilya Bogdanov <fumlead@gmail.com>

Applying @vitvakatu review

c44d0f4

Frizi reviewed May 19, 2023

View reviewed changes

kazcw approved these changes May 19, 2023

View reviewed changes

Frizi mentioned this pull request May 19, 2023

IDE died when editing the node such that no more evaluation was happening #6727

Closed

2 tasks

hubertp self-requested a review May 22, 2023 07:41

farmaazon added the CI: Ready to merge This PR is eligible for automatic merge label May 22, 2023

farmaazon mentioned this pull request May 22, 2023

Enso dying on me while I'm trying to Panic.catch with Invalid Version error #6701

Closed

2 tasks

Merge remote-tracking branch 'origin/develop' into wip/ao/reloading_a…

c15ed09

…fter_desynchronization

hubertp reviewed May 22, 2023

View reviewed changes

wdanilo approved these changes May 23, 2023

View reviewed changes

Merge branch 'develop' into wip/ao/reloading_after_desynchronization

3b25e89

mergify bot merged commit a27f19f into develop May 23, 2023

mergify bot deleted the wip/ao/reloading_after_desynchronization branch May 23, 2023 08:19

hubertp mentioned this pull request May 30, 2023

Adding a single node in the middle of the workflow is desynchronizing IDE and Engine and causing IDE to stop sending the updates without notifying the user #6634

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reloading file in LS after desynchronization. #6752

Reloading file in LS after desynchronization. #6752

farmaazon commented May 18, 2023 •

edited

Loading

farmaazon commented May 18, 2023

vitvakatu left a comment

vitvakatu May 19, 2023

farmaazon May 19, 2023

vitvakatu May 19, 2023

vitvakatu May 19, 2023

farmaazon May 19, 2023

vitvakatu May 19, 2023

Frizi May 19, 2023

farmaazon May 19, 2023

Frizi commented May 19, 2023

hubertp left a comment

wdanilo May 23, 2023

wdanilo May 23, 2023

wdanilo May 23, 2023

wdanilo May 23, 2023

wdanilo May 23, 2023

wdanilo May 23, 2023

Frizi May 23, 2023

wdanilo May 23, 2023

		/// Send to LanguageServer update about received notification about module. Returns the new
		/// content summery of Language Server state.

		/// Listen for all module changes and sends proper updates to the Language Server.
		///

Reloading file in LS after desynchronization. #6752

Reloading file in LS after desynchronization. #6752

Conversation

farmaazon commented May 18, 2023 • edited Loading

Pull Request Description

Important Notes

Checklist

farmaazon commented May 18, 2023

vitvakatu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frizi commented May 19, 2023

QA report 🥦

hubertp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

farmaazon commented May 18, 2023 •

edited

Loading