Skip to content

Don't propagate errors into Node.js - #2926

Merged
mpilquist merged 2 commits into
typelevel:mainfrom
armanbilge:fix/js-stream-destroy-error
Jul 1, 2022
Merged

Don't propagate errors into Node.js#2926
mpilquist merged 2 commits into
typelevel:mainfrom
armanbilge:fix/js-stream-destroy-error

Conversation

@armanbilge

@armanbilge armanbilge commented Jun 14, 2022

Copy link
Copy Markdown
Member

H/t @satabin for reporting in gnieh/fs2-data#335 (comment).

Previously, exceptions encountered in fs2.Stream would also be propagated into the Node.js stream it was interopping with. In v3.2.8 this started crashing the Node.js process with:

node:events:505
      throw er; // Unhandled 'error' event
      ^
Error: unexpected '1' before object key
    at $c_Lfs2_io_internal_ThrowableOps$ThrowableOps.toJSError__sjs_js_Error (/home/runner/work/fs2-data/fs2-data/json/circe/.js/target/scala-2.13/fs2-data-json-circe-test-fastopt/main.js:11048:10)
    at /home/runner/work/fs2-data/fs2-data/json/circe/.js/target/scala-2.13/fs2-data-json-circe-test-fastopt/main.js:11134:92
    ...

(Note: the unexpected '1' before object key is an expected error raised in the fs2-data test suite.)

Propagating the error seems sensible, but probably is not the right thing to do.

  1. It's not necessary to preserve the error, since it's already being handled primarily through the FS2/CE3 control flow, propagation would simply be a side-channel.
  2. The error is likely to be unhandled on the Node.js side, which means it will crash the process. This effectively circumvents FS2/CE3 control flow which is undesirable.

I think this was an existing bug, that became exposed through the changes in #2918. Specifically in 0c71749 besides replacing the facade I swapped in the MicrotaskExecutor for SyncIO (probably should have done it in another PR :) This introduced a new cede/yield due to the evalOn which gave the error event an opportunity to emit when previously it did not.

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

Error: too hot to handle!
    at $c_Lfs2_io_internal_ThrowableOps$ThrowableOps.toJSError__sjs_js_Error (/workspace/fs2/io/js/target/scala-2.13/fs2-io-test-fastopt/https:/raw.githubusercontent.com/typelevel/fs2/9e924ca24ef72d15b09825905ea29b344e4bd67b/io/js/src/main/scala/fs2/io/internal/ThrowableOps.scala:30:34)
    at /workspace/fs2/io/js/target/scala-2.13/fs2-io-test-fastopt/https:/raw.githubusercontent.com/typelevel/fs2/9e924ca24ef72d15b09825905ea29b344e4bd67b/io/js/src/main/scala/fs2/io/ioplatform.scala:74:41
@mpilquist
mpilquist merged commit 7f26024 into typelevel:main Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants