-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Ask clarification about streaming implementation for rpc service #784
Comments
The |
I just came here to create a similar issue (hi @dcodeIO 👋 😄) I'm also confused by the streaming API. Shouldn't the rpc.Service do something different for streaming methods versus regular ones? Looks to me that a service can only have one method that is streaming and you can call the method only once before having to create a new rpc.Service instance. Also I don't think the current implementation will work at all with promises, since once resolved the resolve callback is a no-op. Something like this would make more sense to me: let stream = myService.streamingMethod()
stream.on('data', (obj) => {
console.log(obj) // decoded message
})
stream.on('end', () => console.log('ended'))
stream.on('error', (error) => throw error) Could be done with a simple pseudo-stream interface (like a event emitter with write/end methods) or by using the readable-stream package which is fairly lightweight and very well tested. |
Nice to meet you again ;)
Individual methods do not return a stream, but the service instance's greeter.on("data", function(response, method) {
console.log("data in " + method.name + ":", response.message);
}); Likewise, This is what happens:
Additionally, when This should also work for multiple methods. I must admit, though, that the current implementation is built around the desire of not adding another dependency besides a minimal event emitter and it might not be the greatest API of all time to work with. Edit: I believe that ideally, protobuf.js itself wouldn't offer a service wrapper on its own. An additional package doing solely services might be better suited, and could contain more dependencies because it wouldn't affect the size of the core library. |
Hmm, I still don't see how the API allows for a proper implementation of streams. Take this example: service ImageService {
rpc GetImage (ImageRequest) returns (stream ImageChunk) {}
}
message ImageRequest {
string name = 1;
}
message ImageChunk {
bytes data = 1;
} let service = ImageService(function myRPCImpl(method, request, callback) {
let image = sometransport.createReadStream(request.name)
image.on('data', (chunk) => { callback(null, {chunk}) })
image.on('end', () => { callback(null) })
image.on('error', (error) => { callback(error) })
})
let data = await myService.getImage({name: 'bar.jpg'}) // here I have only the first chunk
myService.on('data', (chunk, method) => {
// here I don't know which image I asked so if I sent multiple getImage requests the chunks would be mixed up
})
// this would throw since myService.rpcImpl is set to null after a stream has finished
// but imagining I create a new service instance for every call I want to make this works:
myService.getImage({name: 'bar.jpg'}, (error, chunk) => {
// even though calling a callback multiple times is considered bad practice
}) IMHO the rpc.Service class needs a bit of a refactor before working with streams become possible. Unless I'm completely misunderstanding how streaming services should work :) |
Ah, you are right, parallel requests using the same method don't work with the current design. |
If you're interested I'd be happy to send a PR with a proposal for a better streaming API |
this makes sense to me as well: let stream = myService.streamingMethod(request)
stream.on('data', (obj) => {
console.log(obj) // decoded message
})
stream.on('end', () => console.log('ended'))
stream.on('error', (error) => throw error) I'd be happy to test/review any proposal 👍 |
@jnordberg Did you come up with a solution ? |
@sulliwane nope, just using non-streaming services for now... |
Would be nice to have streaming working for rpc.Service :) Especially since protobuf.js API is so much nicer to work with than google-protobuf |
any chance to get this going again? |
Hi, is anyone working on this issue? If not I could work on this. Let me know since I need server streaming support. Current Example: proto
typescript generated method public getServerStream(request: debug.IStreamRequest): Promise<debug.IStreamReply>; Instead it should return Observable object or EventEmitter as @sulliwane pointed out. interface Stream {
on(eventName: 'data' | 'end' | 'error', cb: (data) => void): void;
}
public getServerStream(request: debug.IStreamRequest): Stream<debug.IStreamReply>; Also we need separate RCPStreaImp const rcp: RPCStreamImpl = (method, requestData, stream) => {
underlingStream.start(method.name, requestData);
underlingStream.on('data', (data) => stream.emit('data', data));
underlingStream.on('end', () => stream.emit('end'));
underlingStream.on('error', (err) => stream.emit('error', err));
}; PS. RPCImpl doesn't allow to get service and pacakge name. I'm also thinking to change method to return name, service and package names. |
Hi, here is PR for streaming support, let me know what do you think #1115 |
Since I don't know when #1115 will be accepted, here two workarounds, assuming you generated a protobufjs static file and wish to use a bidirectional stream: Variant 1: inject custom RPC method definition into const grpc = require('grpc');
const messages = require('./pbjs-static.js').my.package;
const MyService = exports.MyService = {
myCall: {
path: '/my.package/myCall',
requestStream: true,
responseStream: true,
requestType: messages.MyCallRequest,
responseType: messages.MyCallResponse,
requestSerialize: arg => messages.MyCallRequest.encode(arg).finish(),
responseDeserialize: arg => messages.MyCallResponse.decode(arg)
}
};
const Client = grpc.makeGenericClientConstructor(MyService)
const client = new Client(
grpcServerUrl,
grpc.credentials.createInsecure()
)
// create bidi-stream
var stream = client.myCall();
// listen on incoming data (decoded via protobufjs)
stream.on('data', function(response){
})
// send data (json is encoded via protobufjs)
stream.write({my: "message"}); Variant 2: directly create a bidi stream from const grpc = require('grpc');
const messages = require('./pbjs-static.js').my.package;
const Client = grpc.makeGenericClientConstructor()
const client = new Client(
grpcServerUrl,
grpc.credentials.createInsecure()
)
var stream = client.makeBidiStreamRequest(
'/my.package/myCall',
arg => messages.MyCallRequest.encode(arg).finish(),
arg => messages.MyCallResponse.decode(arg)
);
// listen on incoming data (decoded via protobufjs)
stream.on('data', function(response){
})
// send data (json is encoded via protobufjs)
stream.write({my: "message"}); |
protobuf.js version: 6.7.1
I'm trying to implement streaming RPC function, to create services. But looking at the streaming example, I don't understand how I can emit these events:
from the
rpcImpl
function:My guess is:
callback(null, data)
.callback(null, null)
.callback(error)
.If that's correct, does it also mean that
rpcImpl
will automatically fire events when receiving a streaming method, and fire a promise when receiving a unary method? Or fire both whatever the method?Thank you for the clarification!
The text was updated successfully, but these errors were encountered: