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

Report Visualization Errors #1671

Merged
merged 15 commits into from Apr 21, 2021
Merged

Report Visualization Errors #1671

merged 15 commits into from Apr 21, 2021

Conversation

4e6
Copy link
Contributor

@4e6 4e6 commented Apr 14, 2021

Pull Request Description

close #1557

Replace VisualisationEvaluationError reply to a binary channel with executionContext/visualisationEvaluationFailed notification. Evaluation error can't be sent as a reply because it is a part of a program execution, that happens asynchronously. It should be a notification with an appropriate context (similar to an expression update notification)

Important Notes

API changes:

  • Add a new notification executionContext/visualisationEvaluationFailed

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@4e6 4e6 self-assigned this Apr 14, 2021
@iamrecursion iamrecursion added Category: Backend --breaking Important: a change that will break a public API or user-facing behaviour p-highest Should be completed ASAP labels Apr 14, 2021
@4e6
Copy link
Contributor Author

4e6 commented Apr 15, 2021

Changelog:

  • add: diagnostic info to executionContext/visualisationEvaluationFailed notification and VisualisationExpressionFailed response
  • fix: JSON serialization of error payloads

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good to me

Just please make sure if the tests in ContextRegistryTest and VisualisationOperationsTest are not unnecessarily duplicated - because it seems at first look that some of them are, but I may be missing some nuances.

ctx: RuntimeContext
): PartialFunction[Throwable, Api.ExecutionResult.Diagnostic] = {
case ex: AbstractTruffleException
if Option(ctx.executionService.getLanguage(ex))
Copy link
Member

Choose a reason for hiding this comment

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

It's not directly relevant to this PR (as this was here before), but I was wondering (and it doesn't seem to be documented) - what is the purpose of this guard? It lets through AbstractTruffleExceptions if the language is null or is Enso - so it specifically ignores errors from different languages, unless language is not set at all.
It seems a bit weird to me, because later we look for the stacktrace so it seems maybe we'd want to have only Enso here? So I'm wondering if it wasn't supposed to be exists instead of forall, but I cannot judge that without documentation that would clarify the intent behind this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The guard filters only the Enso errors. But currently, getLanguage can only tell the language by the source file, and return null when the error originates in built-in nodes. It means that we need forall to include builtins as well. Builtins cover a significant part of the language, and most of the calls end up there, so we definitely want to support them.

The guard is definitely not obvious, so I'll add a comment there 👍

Copy link
Member

Choose a reason for hiding this comment

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

The guard filters only the Enso errors. But currently, getLanguage can only tell the language by the source file, and return null when the error originates in built-in nodes.

So I understand that technically we can get a node with null language that is not from Enso, right? But we have no good way to check that? If yes, if we get such input, do we expect that it could crash or is it safe to execute the rest of the code on it? (And if it is safe, why do we ignore other languages then?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be safe to execute the code on any exception. When the code was written, we only had an Enso and a host (Java) languages, and the idea was to provide the right context and show only stuff related to Enso

…job/ProgramExecutionSupport.scala

Co-authored-by: Radosław Waśko <radoslaw.wasko@enso.org>
@4e6 4e6 merged commit d9e1a47 into main Apr 21, 2021
@4e6 4e6 deleted the wip/db/visualization-errors branch April 21, 2021 13:32
iamrecursion pushed a commit that referenced this pull request Apr 28, 2021
Add `executionContext/visualisationEvaluationFailed`
notification
@radeusgd radeusgd mentioned this pull request Oct 28, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--breaking Important: a change that will break a public API or user-facing behaviour p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visualization Errors Are Not Reported
3 participants