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.

Sign up for GitHub

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

Jump to bottom

Eliminate race condition for edit/close/open cmds #6998

Merged
merged 3 commits into from Jun 9, 2023
Merged

Eliminate race condition for edit/close/open cmds #6998

merged 3 commits into from Jun 9, 2023

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Jun 8, 2023 •
edited

Pull Request Description

There was an inherent race condition between edit, close & open commands which could not be prevented solely using locks. EditFileCmd triggered EnsureCompiledJob which was applying edits collected over time. At the same CloseFileCmd and OpenFileCmd were executed asynchronously and required locks on compilation unit and file lock.

Additionally, open file was resetting the module's runtime source irrespective of any edits that could already have been applied with the asynchronous execution in EnsureCompiledJob. This was visible especially during early manipulation of the project when open/close was performed due to a bug in IDE (#6843).

Now commands can be run either synchronously or asynchronously. Only that way can we ensure that close & open commands finish by the time any editions are being applied to module's sources.

Closes #6841.

Important Notes

In the given video, "foo" would be greyed out because it would never be part of the module's (runtime) sources. Therefore no IR would be generated for it or instrumentation, meaning it would be present in expressionUpdates information necessary for IDE.

Kazam_screencast_00014.webm

Checklist

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

  • 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.

4e6
4e6 approved these changes Jun 8, 2023
View reviewed changes
Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

I like this approach 👍

JaroslavTulach
JaroslavTulach approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Approving as harmless, but I don't see what this PR actually changes except doing some stylistic refactorings. How it is supposed to eliminate race condition? That remains a mystery to me.

engine/runtime/src/main/scala/org/enso/interpreter/instrument/command/AsynchronousCommand.scala Outdated Show resolved Hide resolved
engine/runtime/src/main/scala/org/enso/interpreter/instrument/command/SynchronousCommand.scala Outdated Show resolved Hide resolved
...untime/src/main/scala/org/enso/interpreter/instrument/execution/CommandExecutionEngine.scala Outdated Show resolved Hide resolved
@hubertp
Eliminate race condition for edit/close/open cmds
42cfcdf 
There was an inherent race condition between edit, close & open commands
which could not be prevented solely using locks. `EditFileCmd` triggered
`EnsureCompiledJob` which was applying edits collected over time. At the
same `CloseFileCmd` and `OpenFileCmd` were executed asynchronously and
required locks on compilation unit  and file lock.

Additionally, open file was resetting the module's runtime source
irrespective of any edits that could already have been applied with the
asynchronous execution in `EnsureCompiledJob`. This was visible
especially during early manipulation of the project when open/close was
performed due to a bug in IDE (#6843).

Now commands can be run either synchronously or asynchronously. Only
that way can we ensure that `close` & `open` commands finish by the time
any editions are being applied to module's sources.
@hubertp
PR review
400b07f
@hubertp hubertp force-pushed the wip/hubert/6841-edit-close-open-cmds-race-condition branch from d634725 to 400b07f Compare June 9, 2023 09:34
@hubertp
Copy link
Contributor Author

hubertp commented Jun 9, 2023

This PR also helps with the

org.enso.syntax.text.Parser.ParserError: Some(Internal error in parser Could not not deserialize metadata (found 2 metadata sections))
  at org.enso.syntax.text.Parser.splitMeta(Parser.scala:44)

error that people were reporting occasionally.

@hubertp hubertp added the CI: Ready to merge This PR is eligible for automatic merge label Jun 9, 2023
@mergify mergify bot merged commit 5c3d6a7 into develop Jun 9, 2023
26 of 27 checks passed
@mergify mergify bot deleted the wip/hubert/6841-edit-close-open-cmds-race-condition branch June 9, 2023 12:12
@enso-bot enso-bot bot mentioned this pull request Jun 10, 2023
Closed
hubertp added a commit that referenced this pull request Oct 11, 2023
@hubertp
Remove synchronous commands to avoid deadlocks
7c033c8 
This change partially reverts #6998 which introduced synchronous
commands to a) avoid deadlocks b) ensure the correct execution of
close/open commands c) avoid data corruption when applying edits.

Sadly, it now turns out that the possibility of synchronous commands is
at odds with how commands are expected to execute. More importantly,
there are commands comming from Language Server but there are commands
comming from Runtime as well. When combined with context locks it
creates an environment for deadlocks as described in #7898.

This change ensures that commands are always executed asynchronously but
also we can also mark some commands to preserve the order of execution
wrt to their submission. That way no deadlocks arise and there is do
data corruption of the enso code.
hubertp added a commit that referenced this pull request Oct 11, 2023
@hubertp
Remove synchronous commands to avoid deadlocks
34f848d 
This change partially reverts #6998 which introduced synchronous
commands to a) avoid deadlocks b) ensure the correct execution of
close/open commands c) avoid data corruption when applying edits.

Sadly, it now turns out that the possibility of synchronous commands is
at odds with how commands are expected to execute. More importantly,
there are commands comming from Language Server but there are commands
comming from Runtime as well. When combined with context locks it
creates an environment for deadlocks as described in #7898.

This change ensures that commands are always executed asynchronously but
also we can also mark some commands to preserve the order of execution
wrt to their submission. That way no deadlocks arise and there is do
data corruption of the enso code.
hubertp added a commit that referenced this pull request Oct 11, 2023
@hubertp
Remove synchronous commands to avoid deadlocks
2cca0fb 
This change partially reverts #6998 which introduced synchronous
commands to a) avoid deadlocks b) ensure the correct execution of
close/open commands c) avoid data corruption when applying edits.

Sadly, it now turns out that the possibility of synchronous commands is
at odds with how commands are expected to execute. More importantly,
there are commands comming from Language Server but there are commands
comming from Runtime as well. When combined with context locks it
creates an environment for deadlocks as described in #7898.

This change ensures that commands are always executed asynchronously but
also we can also mark some commands to preserve the order of execution
wrt to their submission. That way no deadlocks arise and there is do
data corruption of the enso code.
hubertp added a commit that referenced this pull request Oct 11, 2023
@hubertp
Execute synchronous commands via separate executor
ea36e31 
Rather than using an overly complicated locking mechanism switched to a
`BlockingQueue` and a separate thread pool quaranteeing that commands
are executed in order but they are also non-blocking.

This change partially reverts #6998 which introduced synchronous
commands to a) avoid deadlocks b) ensure the correct execution of
close/open commands c) avoid data corruption when applying edits.

Sadly, it now turns out that the possibility of synchronous commands is
at odds with how commands are expected to execute. More importantly,
there are commands comming from Language Server but there are commands
comming from Runtime as well. When combined with context locks it
creates an environment for deadlocks as described in #7898.
@hubertp hubertp mentioned this pull request Oct 11, 2023
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@4e6 4e6 4e6 approved these changes

@JaroslavTulach JaroslavTulach JaroslavTulach approved these changes

@Akirathan Akirathan Awaiting requested review from Akirathan Akirathan is a code owner

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enso stopped working properly when creating node during Libs compilation
3 participants
@hubertp @4e6 @JaroslavTulach