revelrylabs / elixir-nodejs

An Elixir API for calling Node.js functions
MIT License
214 stars 31 forks source link

Add failing test for console.log statements #44

Closed Zinggi closed 4 years ago

Zinggi commented 4 years ago

Hi there :wave:

Instead of creating an issue, I thought I'll add a failing unit test instead.

When using this library at work, we have the issue that log statements in JS code do crash the NodeJS worker process. In our case, we are using a third party JS library that we don't control and this library sometimes uses log statements, which then kill the caller process.


The crash happens because the library uses stdout for the server protocol https://github.com/revelrylabs/elixir-nodejs/blob/545211106f42c12408cef2091aedb964022fd7f9/priv/server.js#L65

Then, when calling console.log, this output is then thought to be part of the communication protocol and fails to decode here: https://github.com/revelrylabs/elixir-nodejs/blob/545211106f42c12408cef2091aedb964022fd7f9/lib/nodejs/worker.ex#L95


To me it seems like the best way to solve this would be to use a different communication channel for the server process, maybe over a temporary unix socket. But I don't know the best solution yet, maybe you have some other idea?

oohnoitz commented 4 years ago

Yeah, we ran into this as well. Not entirely sure what a good solution is in this case. Most implementations lean towards the usage of stdin/stdout which we've done here. However, like you mentioned, some libraries abuse the usage of console.log statements for various things which ends up causing problems for us.

The stdin/stdout was simple enough here because we spin up the process and don't really keep it alive. Makes it easier to clean up after itself. Changing the protocol would mean we'd have to handle it on both sides and also clean both sides up properly.

The current workaround we've been using in the meantime was to override console.* to write to a file so it continues to function the way we expect it to.

Zinggi commented 4 years ago

One way we could work around this issue would be to prepend an unguessable, arbitrary fixed string to each protocol line, e.g. "__elixirnodejs__UOSBsDUP6bp9IF5__" <> rest. This way it would be easy to ignore console.log statements and other things printing to stdout. It's a bit hacky, but implementing this would be very easy with minimal code changes.

I can update my PR with this solution if you think this would be acceptable. I don't think it's too hacky, as the only way this could break is precisely crafted console.log statements from the js side. What do you think?

Zinggi commented 4 years ago

I implemented my proposed somewhat hacky solution. If you have a better idea, we can still go for another solution.

Zinggi commented 4 years ago

@oohnoitz Great! Are you planning to release the new changes on hex.pm or do you want to bundle them with some other changes first?

oohnoitz commented 4 years ago

@Zinggi yeah, I'll create a release after updating the changelog and a few things.

oohnoitz commented 4 years ago

@Zinggi it's been published as 2.0.0. did a major bump just to be safe with the changes made here.

Zinggi commented 4 years ago

@oohnoitz Great, thanks a lot!