propagating compiler errors #116

arekouzounian · 2024-06-24T21:09:30Z

WIP

Meant to address #115

process now propagates exit code of the command it's running
xhr response on frontend also holds a body field to hold error messages from bad requests (>= 400 status code)

Still working on:

error handling for runtime errors
standardized error structure

arekouzounian · 2024-06-24T22:49:06Z

engine/controller/controller.go

-						ControllerErr: PreRunError,
-						RunOutput:     nil,
-						CommandErr:    nil,
+						ControllerErr: nil,
+						RunOutput:     preRunOut,
+						CommandErr:    commandErr,


Before, the PreRunError was being thrown when compile commands would fail. This would make handling compilation errors different from handling runtime errors; I changed it so that both could be handled the same way.

arekouzounian · 2024-06-24T22:50:22Z

engine/process/main.go

+					cmd.Stderr.Write([]byte(err.Error()))
+
+					// signal exit code: 128 + signal code
+					os.Exit(128 + int(ws.Signal()))


error codes are being captured from the process command, and luckily the error from exec.Command already has a nice explanation of what caused the exit, so I just write that to the command's stderr in the case of a signal, such as a segfault

arekouzounian · 2024-06-24T22:52:30Z

server/api/v2/types.go

@@ -22,5 +22,5 @@ type RunResponse struct {
 	// Stderr is standard error stream from running program
 	Stderr string `json:"stderr"`
 	// Error result from either compilation or runtime errors. Will be nil if both compilation and runtime were successful.
-	Error error `json:"error"`
+	Error string `json:"error"`


One big thing I noticed is that the internal json.Marshal wasn't properly marshalling the error type. If I did something like

fmt.Println("%v", err)

I would get the right error message, but once it was marshaled, it was returned as an object like

{ ... error: {Stderr: null} }

My solution was simply to call the .Error() method on the propagated errors, to turn them into strings before they're marshaled

Makes sense to turn into string. Good catch, sorry I should've noticed that if I was writing this before.

arekouzounian · 2024-06-24T22:54:06Z

web-frontend/js/run-request.js

I added pre blocks for all of these, which helps with compile time errors to be shown properly:

This is actually looking really great! Not sure if I'll get time to review much before Wednesday, but just wanted to leave that initial feedback!

arekouzounian · 2024-06-24T23:01:40Z

@camerondurham I tried to implement an effective solution without modifying too much. The main idea right now is to capture the process exit code as well as the process signal, if any.

I put a bunch of comments all over to explain my thought process; it's not as standardized of an error system as it could be, but I think that it should be able to communicate what's going wrong effectively enough.

Let me know what you think; I'm also fairly sure these changes break a couple of test cases so let me know if you'd like me to change those or anything.

arekouzounian · 2024-06-26T05:35:55Z

forgot to add this screenshot for what it looks like with segfaults:

camerondurham · 2024-06-27T15:40:44Z

This looks awesome! Glad Command includes the failed output in std error response. I think the other failing integration test is just extremely flakey, but will need to take a look into that in a bit. I know you've manually verified this, so I can be okay with merging if that's still failing. But your change to include errors should not be causing anything new to fail.

I can work on fixing the flaky test later anyways.

camerondurham · 2024-06-26T21:17:28Z

server/api/v2/types.go

@@ -22,5 +22,5 @@ type RunResponse struct {
 	// Stderr is standard error stream from running program
 	Stderr string `json:"stderr"`
 	// Error result from either compilation or runtime errors. Will be nil if both compilation and runtime were successful.
-	Error error `json:"error"`
+	Error string `json:"error"`


Makes sense to turn into string. Good catch, sorry I should've noticed that if I was writing this before.

camerondurham · 2024-06-27T00:44:47Z

engine/process/main.go

 	err = cmd.Run()

 	if err != nil {
 		print.ProcDebug("error running process: %v\n", err)
-		os.Exit(ERunningProc)


Linter is complaining about this, not being used, can remove lined 38 as no longer being used? ERunningProc = 20 // error code when running command

camerondurham · 2024-06-27T15:36:20Z

engine/process/main.go

+			if ws, ok := cast.Sys().(syscall.WaitStatus); ok {
+				if ws.Signaled() {
+					// more verbose err message
+					cmd.Stderr.Write([]byte(err.Error()))


This returns a value we may want to sanity check and debug log if it fails. It's unlikely to fail, but we can add visibility there just in case.

arekouzounian · 2024-07-01T18:47:54Z

@camerondurham apologies for the delay on this. My most recent push should fix all the linter issues, I added some more debug statements, got rid of the unused error code, and am passing earthly lints locally. Let me know what else I can do 🙂

camerondurham

Hey, no worries I meant to look at the other test failures.

I finally pushed a commit to fix some outdated tests after changes to pre-run commits.

Honestly the pre-run commit since it'll always be nil should probably be removed all together but I'll not block on that.

camerondurham · 2024-07-02T01:38:07Z

engine/controller/controller.go

-						ControllerErr: PreRunError,
-						RunOutput:     nil,
-						CommandErr:    nil,
+						ControllerErr: nil,


Since this is nil should probably just remove altogether.

arekouzounian added 3 commits June 24, 2024 14:04

propagating compiler errors

f0b2488

fixing mistake with localhost requests

5195b43

runtime errors also propagating

7d58b16

arekouzounian commented Jun 24, 2024

View reviewed changes

update mock

0d15f08

arekouzounian requested a review from camerondurham June 24, 2024 23:01

camerondurham reviewed Jun 27, 2024

View reviewed changes

fix lints

1947b55

Update controller and server tests

933bd24

camerondurham approved these changes Jul 2, 2024

View reviewed changes

camerondurham merged commit b24cff6 into camerondurham:main Jul 2, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

propagating compiler errors #116

propagating compiler errors #116

arekouzounian commented Jun 24, 2024 •

edited

Loading

arekouzounian Jun 24, 2024

arekouzounian Jun 24, 2024 •

edited

Loading

arekouzounian Jun 24, 2024

camerondurham Jun 26, 2024

arekouzounian Jun 24, 2024

camerondurham Jun 25, 2024

arekouzounian commented Jun 24, 2024

arekouzounian commented Jun 26, 2024

camerondurham commented Jun 27, 2024

camerondurham Jun 26, 2024

camerondurham Jun 27, 2024

camerondurham Jun 27, 2024

arekouzounian commented Jul 1, 2024

camerondurham left a comment

camerondurham Jul 2, 2024

propagating compiler errors #116

propagating compiler errors #116

Conversation

arekouzounian commented Jun 24, 2024 • edited Loading

WIP

Choose a reason for hiding this comment

arekouzounian Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arekouzounian commented Jun 24, 2024

arekouzounian commented Jun 26, 2024

camerondurham commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arekouzounian commented Jul 1, 2024

camerondurham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arekouzounian commented Jun 24, 2024 •

edited

Loading

arekouzounian Jun 24, 2024 •

edited

Loading