mockttp icon indicating copy to clipboard operation
mockttp copied to clipboard

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

Open sbougon opened this issue 2 years ago • 4 comments

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 avatar May 24 '23 01:05 sbougon

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() } ?

sbougon avatar May 24 '23 01:05 sbougon

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?

pimterry avatar May 24 '23 14:05 pimterry

@pimterry that would be awesome, thank you

sbougon avatar May 24 '23 19:05 sbougon

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.

pimterry avatar May 25 '23 09:05 pimterry