Send executionComplete response only on success
#7143
Conversation
There was a problem hiding this comment.
I'd expect some change in the protocol document - here is an idea:
Right now the protocol talks about executionFailed and executionComplete being sent from server to the client at "some undefined moment".
Shouldn't the document connect execution request with these two messages and claim that once an execution is started it is guaranteed that either executionFailed xor executionComplete is sent back to the client when the execution finishes?
I think this information is captured in the Runtime*Tests, but it might be useful to spell it out in the protocol specification as well.
...polyglot/src/test/scala/org/enso/interpreter/test/instrument/RuntimeVisualizationsTest.scala
Show resolved
Hide resolved
|
I updated docs. I also replaced some Scala's case classes with case objects, as they should. Jackson needed a bit more configuration to be able to deal with them properly. |
`executionFailed` instead is sent when an evaulation finishes with a a critical failure or a non-critical error. The PR tries to miniminally modify the change in the messages exchange so as to avoid a major redesign at this point. Closes #7002.
Jackson wasn't deserializing properly Scala's (case) objects by creating new instances every time. That's the reason some definitions used (improperly) Scala's case classes instead.
hubertp
force-pushed
the
wip/hubert/7002-execution-complete
branch
from
4785191
to
a159a2a
Compare
Exchanges between IDE and LS are full of `executionFailed` "Execution of function main interrupted" messages. Those are part of regular execution and should be reported as failures.
| mapper.registerModule(DefaultScalaModule) | ||
| mapper | ||
| .registerModule(DefaultScalaModule) | ||
| .registerModule(ScalaObjectDeserializerModule) |
There was a problem hiding this comment.
I wonder, why it is a separate module and not a part of the Scala one.
There was a problem hiding this comment.
Yeah, clearly someone mixed that up
Package's config information, once loaded, never changed. While there is typically no need for it, this was problematic when the config became out-of-sync with the filesystem, like in the case of project rename action. In rename, the config's properties would be updated in the FS, but that would never be reflected in module's package. Therefore further compilations would continue to ask for the old namespace. Most of the changes are cosmetic (s/`.config`/`.getConfig()`) except for the new `reloadConfig` method on `Package` that is being called in `RenameProjectCmd` handler. Closes #7062. # Important Notes The reported `ExecutionFailed` error should have been mostly fixed already via #7143. This change makes sure that all the related warnings are gone as well and the compiler uses the updated namespace.
Pull Request Description
executionFailedinstead is sent when an evaulation finishes with a a critical failure or a non-critical error.The PR tries to miniminally modify the change in the messages exchange so as to avoid a major redesign at this point.
Closes #7002.
Important Notes
Unblocks IDE which will need to modify to this new setup.
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.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build.