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

Empty log line on worker error #945

Labels
external you can do this Good candidate for a pull request.

Comments

@micon4sure
Copy link

micon4sure commented Jan 18, 2020

Error when trying to create faulty worker

Description

When trying to create a new worker that has errors, an empty log line is printed to the console instead of the actual error.

Replication

For replication, see this repository: https://github.com/teh-mICON/ts_worker_error

run via ts-node start.ts or npm run start

Error when directly invoking worker.ts

$ ts-node worker.ts

/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:245
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
worker.ts:2:1 - error TS2304: Cannot find name 'asd'.

2 asd();
  ~~~

    at createTSError (/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:245:12)
    at reportTSError (/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:249:19)
    at getOutput (/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:357:34)
    at Object.compile (/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:415:32)
    at Module.m._compile (/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:493:43)
    at Module._extensions..js (internal/modules/cjs/loader.js:947:10)
    at Object.require.extensions.<computed> [as .ts] (/home/micon/.nvm/versions/node/v12.9.1/lib/node_modules/ts-node/src/index.ts:496:12)
    at Module.load (internal/modules/cjs/loader.js:790:32)
    at Function.Module._load (internal/modules/cjs/loader.js:703:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:999:10)

Output when Invoking start.ts

$ ts-node start.ts

ts-node start.ts  4.97s user 0.86s system 188% cpu 3.084 total

Additional notes

An empty log line is also created on runtime exception, if code compiled correctly

@micon4sure
Copy link
Author

micon4sure commented Jan 18, 2020

Resolved.

Adding a try/catch block in workerAdapter.js solves the issue:

try {
  require('ts-node').register();
  require(path.resolve(workerData.file));
} catch(error) {
  console.error(error)
}

@micon4sure
Copy link
Author

Issue sometimes still persists even with aforementioned fix.

Trying to replicate.

@micon4sure micon4sure reopened this Jan 19, 2020
@micon4sure
Copy link
Author

I replicated the issue. When the error is thrown while inside the parentPort.addListener 'message' context, no error message is shown on the console. I have updated the error repository accordingly: https://github.com/teh-mICON/ts_worker_error/blob/master/worker.ts#L6

Workaround for now is obviously to try/catch the entire context.

@cspotcode
Copy link
Collaborator

This looks like an issue with error handling, not with ts-node. Does the issue occur if your worker.ts is renamed to worker.js (and TypeScript syntax removed) and you do not use ts-node?

@micon4sure
Copy link
Author

No. The basically exact same code with vanilla JS (without the workerAdapter.js in between) works as expected:
https://github.com/teh-mICON/ts_worker_error/blob/master/worker.js#L7

$ nodejs start.js
msg bar

events.js:186
      throw er; // Unhandled 'error' event
      ^
Error: test
    at MessagePort.<anonymous> (/mnt/c/dev/code/ts_worker_error/worker.js:7:9)
    at MessagePort.emit (events.js:209:13)
    at MessagePort.onmessage (internal/worker/io.js:70:8)
Emitted 'error' event on Worker instance at:
    at Worker.[kOnErrorMessage] (internal/worker.js:176:10)
    at Worker.[kOnMessage] (internal/worker.js:186:37)
    at MessagePort.<anonymous> (internal/worker.js:118:57)
    at MessagePort.emit (events.js:209:13)
    at MessagePort.onmessage (internal/worker/io.js:70:8)

@cspotcode
Copy link
Collaborator

cspotcode commented Jan 20, 2020

I narrowed it down to an issue in source-map-support, one of our dependencies.

https://repl.it/@AndrewBradley/ts-nodeissues945

When source-map-support is installed in the worker, the worker doesn't emit error events to the main thread properly.

If this makes sense, you can file an issue at https://github.com/evanw/node-source-map-support/issues

EDIT: this code might be the culprit: https://github.com/evanw/node-source-map-support/blob/master/source-map-support.js#L475-L486

@micon4sure
Copy link
Author

micon4sure commented Jan 20, 2020

Well, I guess I could but it would actually make more sense if you did it since you could give a much better description of the actual problem. I'm not really sure how the problem arises from the source maps or how to provide steps to reproduce. (Other than the one given in this post which is not very helpful I think)

@cspotcode
Copy link
Collaborator

cspotcode commented Jan 20, 2020 via email

@cspotcode cspotcode added external you can do this Good candidate for a pull request. labels Feb 4, 2020
addaleax added a commit to addaleax/node-source-map-support that referenced this issue Apr 19, 2020
Node.js Worker threads don’t handle `uncaughtException` the same way
that the main thread does. The shim does not apply/make sense here,
but breaks error handling support in unexpected ways instead.

Fixes: evanw#268
Fixes: TypeStrong/ts-node#945
addaleax added a commit to addaleax/node-source-map-support that referenced this issue Apr 19, 2020
Node.js Worker threads don’t handle `uncaughtException` the same way
that the main thread does. The shim does not apply/make sense here,
but breaks error handling support in unexpected ways instead.

Fixes: evanw#268
Fixes: TypeStrong/ts-node#945
@addaleax
Copy link

Just ran into this too, would be cool if you could verify that evanw/node-source-map-support#269 does fix the issue as described in #945 (comment) (and it fixes a similar issue for me locally).

@LinusU
Copy link

LinusU commented Apr 19, 2020

Merged the PR from @addaleax and published as 0.5.17, feel free to try it and report if it work/doesn't work 🚀

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