fix: Re-add support for clientError listeners #1897
Conversation
In the recent fix #1895, the `clientError` event was removed from the list of events proxied from the underlying server object to the restify Server. That fix was needed because merely adding a listener to the underlying server for the `clientError` event prevents the default behavior of returning an appropriate error code and closing the socket. So by setting up a listener to proxy the event we were preventing that default behavior, leading to hanging/leaked requests. But! We should still support proxying `clientError` events if restify users have added a listener for it. This enables logging or other custom behavior. Restify users adding a `clientError` listener take on the responsibility of sending an appropriate error and closing the socket, exactly as noted in the Node.js docs for the `clientError` event. https://nodejs.org/docs/latest-v16.x/api/http.html#event-clienterror This change ensures we add a listener for `clientError` to the underlying server object if and only if a listener for `clientError` has been added to the restify server.
| }).end(); | ||
| }); | ||
|
|
||
| test('Server correctly handles multiple clientError listeners', function(t) { |
There was a problem hiding this comment.
Not sure about this. So it is an event emitter, so obviously two handlers is technically valid, but the use case here is almost certainly to be sending a response in some cases, which shifts the burden onto each handler to make sure it properly checks before sending the response and closing the socket. If a user does this in multiple handlers they could get some pretty unexpected error results if they are not careful.
Could a better user experience from restify be to error or warn when they try to add a second handler?
There was a problem hiding this comment.
@wesleytodd, I agree generally. This is more of a contrived test rather than a valid usage example. I added this test to make sure that the proxying is done correctly / doesn't emit the proxied event multiple times if multiple listeners are added.
My goal with this change was to restore the ability to add a clientError listener after #1895 removed it, so am matching the original behavior here. I don't think folks would do this very often if at all. One actual use case might be to do the response in one handler and log in another. I'm not opposed to adding a warning but feels beyond the scope of this change.
There was a problem hiding this comment.
cool, yeah makes sense. I just wanted to bring up this as I have thought before that sometimes these kind of api's from node core have foot guns, and I think frameworks are generally good places to shore up those gaps. I for sure don't think this one is a common issue worth spending much time on. So 👍 to the change!
|
Naive Q - would it make sense for restify to replicate the default behavior when you attach a listener? Or is this a pretty standard Node.js expectation? It just seems strange that adding a listener would actually change behavior. |
@DonutEspresso Here is the default handler logic that is executed when no listeners are registered for We could copy that default behavior into restify, but that feels like coupling more tightly with the underlying implementation than just conditionally proxying the event. If that default implementation in Node.js ever changed, we'd likely need to change it in restify as well anyway. |
In the recent fix #1895, the
clientErrorevent was removed from thelist of events proxied from the underlying server object to the restify
Server. That fix was needed because merely adding a listener to the
underlying server for the
clientErrorevent prevents the defaultbehavior of returning an appropriate error code and closing the socket.
So by setting up a listener to proxy the event we were preventing that
default behavior, leading to hanging/leaked requests.
But! We should still support proxying
clientErrorevents if restifyusers have added a listener for it. This enables logging or other custom
behavior. Restify users adding a
clientErrorlistener take on theresponsibility of sending an appropriate error and closing the socket,
exactly as noted in the Node.js docs for the
clientErrorevent.https://nodejs.org/docs/latest-v16.x/api/http.html#event-clienterror
This change ensures we add a listener for
clientErrorto theunderlying server object if and only if a listener for
clientErrorhasbeen added to the restify server.