[GitHub] [daffodil-vscode] mbeckerle commented on a diff in pull request #666: Remove blocking dialogs for debugger errors

["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