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

console.warn() in websocket-handlers.js is poluting the stdout #148

Open
sbougon opened this issue May 24, 2023 · 4 comments
Open

console.warn() in websocket-handlers.js is poluting the stdout #148

sbougon opened this issue May 24, 2023 · 4 comments

Comments

@sbougon
Copy link

sbougon commented May 24, 2023

Hello,

love your library, thank you !

I developed a CLI using mockttp, but every so often, I see the following on my program stdout:
Error: read ECONNRESET at TCP.onStreamRead (node:internal/stream_base_commons:217:20) at TCP.callbackTrampoline (node:internal/async_hooks:130:17) { errno: -54, code: 'ECONNRESET', syscall: 'read' }

Turns out it's because an error is thrown (correctly and can be handled by my code (using server.on('abort',...)) but because the code in https://github.com/httptoolkit/mockttp/blob/main/src/rules/websockets/websocket-handlers.ts#L395 calls console.warn(e), I can't do anything to provide a clean output to my program. The only way would be to hot-patch console.warn, but I don't want to do this.

In my case, I'm proxying an Android app which repeatedly tries to see if a debugging tools will accept a connection. When there is not debugging tool present, the socket errors out (✅) but a console.warn() gets printed (❌).

Would you be willing to accept a PR which remove the console.warn() ?

@sbougon
Copy link
Author

sbougon commented May 24, 2023

In other words, in my use-case, that code path is expected, and so the warning is not useful.

Would you be okay to trade the console.warn() for a if (this.debug) { console.log() } ?

@pimterry
Copy link
Member

I'm open to fixing this. Unfortunately you can't use the existing debug configuration though, as it's only available in the top-level Mockttp server itself, not in handlers within individual rules. It could be possible to pass that all the way through, distributing some kind of 'rule context' everywhere, but it's structurally a bit messy.

One approach that might be interesting would be to add a configuration property to control this for all passthrough rules (both requests & websockets). Something like logUpstreamErrors: <bool>, defaulting to true, but which you could set to false in your scenario, like:

server.forAnyWebSocket().thenPassThrough({
    logUpstreamErrors: false
});

I think true is a sensible default - for most people, when proxying traffic they expect it all to work, and cases where there are low-level failures are notable, but it's reasonable to want to ignore that if you know those will be frequent and/or inconvenient.

Would that work for you?

@sbougon
Copy link
Author

sbougon commented May 24, 2023

@pimterry that would be awesome, thank you

@pimterry
Copy link
Member

Ok, great! As an approach that works for me too. If you're interested in this, I'd happily accept a PR in that direction.

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

No branches or pull requests

2 participants