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

Improve error handling in watch mode when dat file can't be read (or emcc fails) #260

Closed
chrispcampbell opened this issue Oct 8, 2022 · 1 comment · Fixed by #434 or #435
Closed

Comments

@chrispcampbell
Copy link
Contributor

The build package has error handling in place already so that errors are caught and a message is displayed appropriately. However, there are still a couple cases where the build process exits while in watch mode when some error is encountered, for example if a dat file can't be read, or emcc/clang fails. We need to fix those cases so that the build process stays running and the error messages are reported correctly.

@chrispcampbell
Copy link
Contributor Author

There were a few overlapping things going on here, so I decided to address them all at once. The net effect of all these fixes is that the local development server (sde dev) will now remain running (won't be terminated prematurely like before) when errors bubble up that weren't previously being handled.

The main thing that was lacking related to reading dat files is that the readDat function didn't handle error events, which caused the errors to bubble up in a way that they weren't being caught by the existing error handling code.

The other central fix was to remove the few remaining places where errors were being caught at a lower level and handled by calling process.exit, which is problematic when you want to have the process stay alive like in the case of sde dev (local development mode).

Side note: sde dev and sde bundle currently have an awkward thing where they spawn a separate child process to run the sde generate step partially due to those process.exit calls. Now that those calls have been removed, I'll have a better chance of fixing the code to make direct calls instead of spawning a process, but that'll be done in a separate PR later.

There were a few other related fixes that are detailed below.

  1. build-once.ts: Expanded the scope of the try/catch so that if a plugin throws an error in preGenerate it will be caught and handled properly.

  2. gen-model.ts: Fixed to throw an error with a custom message that is more clear than the default one thrown in spawnChild.

  3. sde-generate.js: Added try/catch so that we get better error messages. Before we would get the default yargs error handling, which shows a vague message followed by a more typical error message. Now we get only the typical error message and stack trace.

before:

$ sde generate --genc arrays_varname.mdl
file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/generate/code-gen.js:41
    throw new Error('Failed to read model')
          ^

Error: Failed to read model
    at Object.generate (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/generate/code-gen.js:41:11)
    at generateCode (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/generate/code-gen.js:12:43)
    at parseAndGenerate (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/parse-and-generate.js:71:14)
    ...
Node.js v20.11.0

after:

$ sde generate --genc arrays_varname.mdl
Error: Failed to read model
    at Object.generate (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/generate/code-gen.js:41:11)
    at generateCode (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/generate/code-gen.js:12:43)
    at parseAndGenerate (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/parse-and-generate.js:71:14)
  1. sde-bundle.js, sde-dev.js: Fixed both of these to log the full standard error. Before we would only log the error, but the stack trace (which is often very helpful) was being suppressed. Now we get the full error message and stack trace.

  2. code-gen.js: Removed the try/catch and abend call here. This allows errors to be caught and handled at a higher level, instead of triggering a process.exit.

  3. helpers.js: Removed the now unused abend function.

  4. read-dat.js: Added an on('error') handler that rejects the promise. Without this, we were getting an "unhandled error event" message that obscured the actual error (and the stack trace did not make it clear where the error was being thrown from). Now we get a more standard looking error message. The stack trace isn't much better, but this will be improved by the next PR that replaces our use of byline with Node built-ins.

before:

node:events:496
      throw er; // Unhandled 'error' event
      ^

Error: ENOENT: no such file or directory, open '/Users/campbell/Projects/CI/sdeverywhere/models/extdata/extdata_data.datfoo'
Emitted 'error' event on ReadStream instance at:
    at emitErrorNT (node:internal/streams/destroy:169:8)
    at emitErrorCloseNT (node:internal/streams/destroy:128:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21) {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/campbell/Projects/CI/sdeverywhere/models/extdata/extdata_data.datfoo'
}

Node.js v20.11.0

after:

Error: Failed to read dat file: ENOENT: no such file or directory, open '/Users/campbell/Projects/CI/sdeverywhere/models/extdata/extdata_data.datfoo'
    at ReadStream.<anonymous> (file:///Users/campbell/Projects/CI/sdeverywhere/packages/compile/src/_shared/read-dat.js:45:14)
    at ReadStream.emit (node:events:518:28)
    at emitErrorNT (node:internal/streams/destroy:169:8)
    at emitErrorCloseNT (node:internal/streams/destroy:128:3)
    at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
  1. read-dat.spec.ts and gen-code-c.spec.ts: Added new unit tests for better coverage of error handling.

  2. model.js: Removed try/catch and process.exit calls. These now throw errors that can be handled properly at a higher level.

  3. toposort.js: Improved the error message slightly.

  4. plugin-wasm/src/plugin.ts: Fixed to throw an error with a custom message that is more clear than the default one thrown in spawnChild.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment