You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by "shanedell (via GitHub)" <gi...@apache.org> on 2023/06/16 13:09:39 UTC
[GitHub] [daffodil-vscode] shanedell opened a new pull request, #666: Remove blocking dialogs for debugger errors
shanedell opened a new pull request, #666:
URL: https://github.com/apache/daffodil-vscode/pull/666
Remove blocking dialogs for debugger errors
- When some errors happen in the debugger they would cause a large blocking dialog in the extension.
- This didn't looks good and didn't allow you to quickly fix issues.
- The errors now show in the terminal and small message in the bottom right corner.
- The message will state the debugger crashed with some information as to why. Then states to look at debugger output for more information.
- Created a new case class for error events to relay back to the extension.
- Update extension to catch error events relayed from the debugger.
- Create class for handling debugEvents so we don't have too many places changing onDidReceiveDebugSessionCustomEvent, on when absolutely necessary like in the hexview.
Closes #649
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell merged pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell merged PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232612467
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -67,9 +65,9 @@ object DAPSession {
def sendEvent(event: DebugEvent): IO[Unit] =
Logger[IO].info(show"<E $event") *> IO.blocking(server.sendEvent(event))
- def terminate(message: String): IO[Unit] =
- sendEvent(new ErrorEvent(s"${message}. check debugger output for more information.")) *>
- sendEvent(new Events.TerminatedEvent())
+ /** Send DebugEvent back to extension and exit session, ending debug */
+ def abort(event: DebugEvent): IO[Unit] =
Review Comment:
yeah that is correct. they all caused the program to stop. I will remove the message param from all ErrorEvents, that makes a look of sense. I will make case objects instead of case classes as well.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -247,7 +247,7 @@ class DAPodil(
)
)
}
- case _ => session.terminate("failed to load sources")
+ case _ => session.abort(new ErrorEvents.SourceError("failed to load sources"))
Review Comment:
I will remove the message param from all ErrorEvents, that makes a look of sense. I will make case objects instead of case classes as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232515730
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -49,8 +49,11 @@ trait DAPSession[Req, Res, Ev] {
def sendResponse(response: Res): IO[Unit]
def sendEvent(event: Ev): IO[Unit]
+ def terminate(message: String): IO[Unit]
}
+case class ErrorEvent(message: String) extends Events.DebugEvent("daffodil.error")
Review Comment:
Created `ErrorEvents` object that will have different error events that can be thrown. Made these more specific to the error that will get thrown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232330524
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -205,8 +212,8 @@ class DAPodil(
debugee(request) match {
case Left(errors) =>
state.set(DAPodil.State.FailedToLaunch(request, errors, None)) *>
- Logger[IO].warn(show"error parsing launch args: ${errors.mkString_(", ")}") *> session
- .sendResponse(request.respondFailure(Some(show"error parsing launch args: ${errors.mkString_(", ")}")))
+ Logger[IO].error(show"error parsing launch args: ${errors.mkString_(", ")}") *>
Review Comment:
You may want the separator to contain a newline so that giant long lines (which each error message already is by itself) don't run together to further compound the super long line problem.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -49,8 +49,11 @@ trait DAPSession[Req, Res, Ev] {
def sendResponse(response: Res): IO[Unit]
def sendEvent(event: Ev): IO[Unit]
+ def terminate(message: String): IO[Unit]
Review Comment:
There are so many reasons the server could terminate. Is this rich enough? Passing just a string back?
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -63,6 +66,10 @@ object DAPSession {
def sendEvent(event: DebugEvent): IO[Unit] =
Logger[IO].info(show"<E $event") *> IO.blocking(server.sendEvent(event))
+
+ def terminate(message: String): IO[Unit] =
Review Comment:
Can you provide scaladoc here about exactly the purpose of the terminate call?
Ex: Is this to be called for normal termination or abnormal termination? Maybe you should have an abend(msg) or abort(message) or abort(throwable) which are reserved for abnormal termination. Having a separate call makes the code more self-documenting, and lets the message strings ignore having to describe abnormal situations.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -382,10 +387,10 @@ class DAPodil(
.variables(DAPodil.VariablesReference(args.variablesReference))
.fold(
Logger[IO]
- .warn(
+ .error(
show"couldn't find variablesReference ${args.variablesReference} in stack ${data}"
) *> // TODO: handle better
- session.sendResponse(request.respondFailure())
+ session.terminate(s"couldn't find variablesReference ${args.variablesReference} in stack ${data}")
Review Comment:
Should this actually ever happen, assuming the system is working properly? Session.terminate seems like a pretty heavy handed response to an inquiry about a variable that doesn't exist, but it is warranted if the client already obtained a list of all the variables earlier and should not be asking about non-existing variables.
I'm having trouble distinguishing the normal back and forth of server request/response which has to talk about "errors" meaning DFDL processing situations, and distinghishing that from defensive coding where the server is just trying to gracefully exit, providing context. The latter really wants to be abstracted down to single-line checks, otherwise it is too much code clutter and people won't want to put it in.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -49,8 +49,11 @@ trait DAPSession[Req, Res, Ev] {
def sendResponse(response: Res): IO[Unit]
def sendEvent(event: Ev): IO[Unit]
+ def terminate(message: String): IO[Unit]
}
+case class ErrorEvent(message: String) extends Events.DebugEvent("daffodil.error")
Review Comment:
Scaladoc please: what's an ErrorEvent for? You should distinguish which of these is this for?
- An unexpected bug in our code (some throwable got thrown)
- A code invariant check failure (which is also a bug in our code, just one we're checking for)
- Data doesn't match schema - A parse error which propagates to top level, causing the parse to fail.
- Data doesn't match schema - A parse error which is causing backtracking (these need to be visible as people are debugging even though they are often suppressed subsequently)
- Infoset doesn't match schema (an unparse error - these are always fatal)
- Schema-definition error (some of these are detected at runtime, most at schema compilation time - the runtime ones are always fatal to a parse/unparse. The compile time ones, well the execution hasn't started yet. )
Daffodil error classes let you distinguish these. I expect the GUI needs to preserve that distinction and cause different GUI behaviors, so this shouldn't just be in the human crafted part of the text of message strings, but a structured prefix of it that can be reliably parsed. (Ex: the error class name starts the message being sent back to the client, or something like that).
##########
src/adapter/daffodilEvent.ts:
##########
@@ -0,0 +1,38 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as vscode from 'vscode'
+import * as fs from 'fs'
+
+import * as daf from '../daffodil'
+import { ensureFile, tmpFile } from '../utils'
+
+export function handleDebugEvent(e: vscode.DebugSessionCustomEvent) {
+ switch (e.event) {
+ case daf.infosetEvent:
+ let update: daf.InfosetEvent = e.body
+ let path = ensureFile(tmpFile(e.session.id))
+ fs.copyFileSync(path, `${path}.prev`)
+ fs.writeFileSync(path, update.content)
+ break
+ case daf.errorEvent:
+ let error: daf.ErrorEvent = e.body
Review Comment:
Ok, so for now we're just treating all errors as fatal, and getting the output out to the user somehow.
But is "debugger crashed" really the only situation here? A parse error propagating to top level isn't a crash of anything. It's normal for a parse to end that way on bad data.
I think you need to distinguish parse errors (and someday unparse errors) as well as schema definition errors (which can be detected at runtime) from other "real" crashes.
For today, as part of getting 1.3.0 released, I'd be happy if the errors at least were distinguished by error types and not just "debugger crashed" when they are often "normal" ends to a parse that has simply detected that the data doesn't match the schema.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -358,11 +363,11 @@ class DAPodil(
_ <- data.stack
.findFrame(DAPodil.Frame.Id(args.frameId))
.fold(
- Logger[IO].warn(
+ Logger[IO].error(
Review Comment:
Can you introduce a one-liner method call to replace all these 5 lines of error-checking and handling?
This looks like an abend to me. You are logging the error. So it appears this is never supposed to happen, i.e., one is technically always supposed to find scopes for the frames.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -399,7 +404,7 @@ class DAPodil(
for {
variable <- launched.debugee.eval(args)
_ <- variable match {
- case None => session.sendResponse(request.respondFailure())
+ case None => session.terminate("eval failed")
Review Comment:
I guess I need to better understand what session.terminate(...) really means.
To me, I think nothing should terminate the session ever, except some sort of bug in our code on one side or the other.
For example, if a parse ends in a parse error, I would expect the UI to continue to support the user interacting with the server to poke around in the state more, examine variables, understand what is on the stack, etc. They can't continue to step the parse any more, but why should the server session terminate?
Has everything in the state already been brought across to the client so that the server really truly can't be relevant any longer?
Is there code-comments or other doc about what are the conditions where the session is supposed to stay intact, and what are the conditions where it is supposed to terminate normally?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232613218
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends DebugEvent("daffodil.error.unexpected")
+ case class UnhandledRequest(message: String) extends DebugEvent("daffodil.error.requestunhandled")
+ case class LaunchArgsParseError(message: String) extends DebugEvent("daffodil.error.launchargparse")
Review Comment:
I will remove the message param from all ErrorEvents, that makes a look of sense. I will make case objects instead of case classes as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232618484
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends DebugEvent("daffodil.error.unexpected")
+ case class UnhandledRequest(message: String) extends DebugEvent("daffodil.error.requestunhandled")
+ case class LaunchArgsParseError(message: String) extends DebugEvent("daffodil.error.launchargparse")
Review Comment:
@mbeckerle so would something like `vscode.window.showErrorMessage(`debugger ${e.event}`)` be enough? It would display something like `debugger daffodil.error.request` in the bottom right. then the error information is inside of the terminal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232514717
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -399,7 +404,7 @@ class DAPodil(
for {
variable <- launched.debugee.eval(args)
_ <- variable match {
- case None => session.sendResponse(request.respondFailure())
+ case None => session.terminate("eval failed")
Review Comment:
This might be something we should support in the future. Because currently even without the terminate when it send `respondFailure` the session is ended and the terminal is stopped. So this might be something to look into.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232437934
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -382,10 +387,10 @@ class DAPodil(
.variables(DAPodil.VariablesReference(args.variablesReference))
.fold(
Logger[IO]
- .warn(
+ .error(
show"couldn't find variablesReference ${args.variablesReference} in stack ${data}"
) *> // TODO: handle better
- session.sendResponse(request.respondFailure())
+ session.terminate(s"couldn't find variablesReference ${args.variablesReference} in stack ${data}")
Review Comment:
So in this instance should this just be a warn in the log and not cause the debugger to exit?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232828085
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends DebugEvent("daffodil.error.unexpected")
+ case class UnhandledRequest(message: String) extends DebugEvent("daffodil.error.requestunhandled")
+ case class LaunchArgsParseError(message: String) extends DebugEvent("daffodil.error.launchargparse")
Review Comment:
@mbeckerle The above comment is what I latest commit does, let me if you would prefer something different.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232830673
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -399,7 +404,7 @@ class DAPodil(
for {
variable <- launched.debugee.eval(args)
_ <- variable match {
- case None => session.sendResponse(request.respondFailure())
+ case None => session.terminate("eval failed")
Review Comment:
@mbeckerle That makes sense to me, thanks for this information. I have created issue https://github.com/apache/daffodil-vscode/issues/671 from this comment. Feel free to add to the ticket if you thing anything is missing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232419882
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -63,6 +66,10 @@ object DAPSession {
def sendEvent(event: DebugEvent): IO[Unit] =
Logger[IO].info(show"<E $event") *> IO.blocking(server.sendEvent(event))
+
+ def terminate(message: String): IO[Unit] =
Review Comment:
@mbeckerle So `terminate` is for stopping the debugger when an error has occurred. I can see why this should be renamed to abend or abort. Maybe should be moved somewhere instead of session since is causing the debugger to terminate or abort instead of the session?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232598964
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -399,7 +404,7 @@ class DAPodil(
for {
variable <- launched.debugee.eval(args)
_ <- variable match {
- case None => session.sendResponse(request.respondFailure())
+ case None => session.terminate("eval failed")
Review Comment:
Yeah, you got it. That was my point: A parse error occurring may well want to stop things from terminating and allow various client/server interactions by the user to better understand the failure.
The ability to move backward in time is typical - when we debug using just trace output from the CLI debugger scrolling backward to see what happened before the most recent error is definitely commonplace. Particularly important for debugging with bigger data files. We debug against files with hundreds of records/messages in them. A failure may result in wanting to look backward at successful parses that happened well before the record/message causing the specific failure. Often the specific error isn't really relevant because the parse went off the rails well before the specific thing that ultimately created the parse error that ended the parse. The evidence of it going off the rails is what you want to find.
In the GUI debugger, some analog to this has to be available. Breakpoints and stepping help, but don't entirely eliminate the need for this.
Certainly an unparse error would not want to cause termination of the server session. There's so much state to interact with in the unparser it's really much richer.
Eventually I'd expect the server session to remain operating indefinitely and an explicit command would be sent over telling the server to restart a new parse/unparse, and only at that point would the state of the current action be lost and things be reset. Even closing the client should send a command to the server telling it to shut-down in orderly fashion.
Once started, I suspect that only an abort within the server, loss of connection to the client (due to client abort/crash), or an explicit shutdown command should end the server session.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] shanedell commented on pull request #666: Remove blocking dialogs for debugger errors
Posted by "shanedell (via GitHub)" <gi...@apache.org>.
shanedell commented on PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#issuecomment-1594991266
@mbeckerle You commented a lot on the areas on where I run a `.terminate`. So even without that and using the `respondFailure` would cause the session to end and the debugger to exit. So here that is basically what `.terminate` is doing but without causing the extension to show a blocking dialog. I would be okay with renaming `terminate` to `abort` and having it take a `event: DebugEvent`. I also can make more specific debug event classes in both the debugger and extension so they are more verbose.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #666: Remove blocking dialogs for debugger errors
Posted by "mbeckerle (via GitHub)" <gi...@apache.org>.
mbeckerle commented on code in PR #666:
URL: https://github.com/apache/daffodil-vscode/pull/666#discussion_r1232536751
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -247,7 +247,7 @@ class DAPodil(
)
)
}
- case _ => session.terminate("failed to load sources")
+ case _ => session.abort(new ErrorEvents.SourceError("failed to load sources"))
Review Comment:
Think about whether you need the string or not. It's already a ErrorEvents.SourceError. Maybe the constructor for that doesn't need a string argument. The ErrorEvents sub classes that are specific enough may not require any further elaboration.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -367,7 +366,7 @@ class DAPodil(
s"couldn't find scopes for frame ${args.frameId}: ${data.stack.frames
.map(f => f.id -> f.stackFrame.name)}"
) *>
- session.terminate(s"couldn't find scopes for frame ${args.frameId}")
+ session.abort(new ErrorEvents.ScopeNotFoundError(s"couldn't find scopes for frame ${args.frameId}"))
Review Comment:
Here you chose to make the log message different from the string passed to the constructor. I'm not sure that's worth it. It's better to make just one string, or no strings at all, and let session.abort do the logging as well to reduce the clutter.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -67,9 +65,9 @@ object DAPSession {
def sendEvent(event: DebugEvent): IO[Unit] =
Logger[IO].info(show"<E $event") *> IO.blocking(server.sendEvent(event))
- def terminate(message: String): IO[Unit] =
- sendEvent(new ErrorEvent(s"${message}. check debugger output for more information.")) *>
- sendEvent(new Events.TerminatedEvent())
+ /** Send DebugEvent back to extension and exit session, ending debug */
+ def abort(event: DebugEvent): IO[Unit] =
Review Comment:
So by just changing this from terminate to abort are you saying all of these usages of session.terminate were aborts indicating some unexpected failure?
If that's the case fine. One test is that the string in the message of an abort would normally never be something a user would see, or need to read, or would need internationalization, etc. They might see it as part of them filing a bug report, but it's not normal for it to be displayed to a user.
##########
src/daffodil.ts:
##########
@@ -54,7 +54,14 @@ export interface BuildInfo {
scalaVersion: string
}
-export const errorEvent = 'daffodil.error'
+export const errorEvents = {
+ 'daffodil.error.unexpected': 'unexpected error',
Review Comment:
These are aborts, so really this doesn't need to be this pretty. Just printing the original case class name coming over from the scala code would be fine.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/DAPodil.scala:
##########
@@ -212,8 +210,10 @@ class DAPodil(
debugee(request) match {
case Left(errors) =>
state.set(DAPodil.State.FailedToLaunch(request, errors, None)) *>
- Logger[IO].error(show"error parsing launch args: ${errors.mkString_(", ")}") *>
- session.terminate(show"error parsing launch args: ${errors.mkString_(", ")}")
+ Logger[IO].error(show"error parsing launch args: ${errors.mkString_("\n")}") *>
Review Comment:
Make session.abort do the logging also. That would condense this error stuff to just one line.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends DebugEvent("daffodil.error.unexpected")
Review Comment:
Since these are case classes, you can drop use of "new" before their names in the code that constructs them.
That's optional in my book. Some people prefer to keep the "new", and I'm fine with that.
##########
server/core/src/main/scala/org.apache.daffodil.debugger.dap/ErrorEvents.scala:
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.daffodil.debugger.dap
+
+import com.microsoft.java.debug.core.protocol.Events.DebugEvent
+
+/** Case classes for errors that we want to relay back to the extension */
+object ErrorEvents {
+ case class UnexpectedError(message: String) extends DebugEvent("daffodil.error.unexpected")
+ case class UnhandledRequest(message: String) extends DebugEvent("daffodil.error.requestunhandled")
+ case class LaunchArgsParseError(message: String) extends DebugEvent("daffodil.error.launchargparse")
Review Comment:
DebugEvent really doesn't need this string arg. You can just do
```
lazy val event = Misc.getNameFromClass(this)
```
since you just need to display these strings as part of the abort info on the client side. The class name will do just fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org