grpc
grpc-js: end event emitted on client streaming errors
Problem description
If an uncaught error occurs in the data handler the end handler is still fired. I'm not sure if this is intentional but based on the standard node.js readable stream I would expect this not to be called.
In this scenario because the callback is already handled here you cannot subscribe to the error event to handle the error and send a response.
clientStreamExample(call, callback) {
call.on('data', () => {
throw new Error('my error');
});
call.on('end', () => {
console.log('ended');
callback(null, {});
});
}
Environment
- OS name, version and architecture: macOS 10.15.4
- Node version: v12.16.2
- Package name and version @grpc/[email protected]
The grpc-js library does not directly emit the end event or handle that error you are throwing, so I think that behavior is coming from the Node stream implementation.
I believe you can add your own handler to the error event even if it already has a handler. However, once an error event has been emitted, grpc-js automatically responds with an INTERNAL error and considers the stream broken, so you wouldn't be able to respond with your own error.
@murgatroid99 after looking further it looks like grpc-js is catching that error here. The error is only being emitting to the ServerReadableStreamImpl stream, the underlying Http2ServerCallStream stream is still continuing and firing the end event.
My issue is that if an error occurs in the data handling event the end handler is always fired and there is no way to tell if the read completed successfully.
const fs = require('fs');
const rr = fs.createReadStream('foo.txt');
rr.on('readable', () => {
throw new Error('my error')
console.log(`readable: ${rr.read()}`);
});
rr.on('end', () => {
console.log('end');
});
In the basic example above the end event wouldn't be fired so I think it would fair to assume that it shouldn't be fired in the client streaming example above.
I see what you're saying now. I think there's a reasonably easy fix for this.
@murgatroid99 thank you for jumping on this so quickly.
Just for reference I also tested consuming the call using async iterators.
async exampleClientStream(call, callback) {
const messages = [];
try {
for await (const request of call) {
const msg = request.getMsg();
messages.push(msg);
throw new Error('error');
}
const reply = new Reply();
reply.setMessage(messages.join(', '));
callback(null, reply);
} catch (err) {
// make changes to error...
callback(err, null);
}
}
This works nicely if you want to make changes to the callback on error because the errors are not propagated to the error event.
On further examination, I don't actually know how to fix this. The relevant code is kind of convoluted.
I was actually just about to reply and say I tested the changes from your pull request and after throwing an error in the data handler for a client streaming request there were now no responses at all.
I agree it is quite difficult to see where the errors are being handled. The reason why I ran into it in the first place was that I was creating a wrapping function that takes in a service implementation and adds opentelemetry spans to it. I was trying to capture all possible errors so that the spans could be ended correctly.
Based on all the buffering logic in the Http2ServerCallStream class it might be better to use a transform stream to handle the connection between it and the method handlers. That plus the stream.pipeline might make it easier to handle errors, but I realise that would be quite a big change.
In the meantime I'm using async iterators as a workaround but this issue might cause some unforeseen problems for anyone porting across a service implementation from grpc to @grpc/grpc-js. Because the end handlers are likely to contain some kind of action that is only meant for successful requests.
Looks like this issue is quite old, but I would like to check if there's any headway with this? I'm facing the same issue here, where the end event is being emitted after the error event is emitted.
