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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
