GRPC::Cancelled should be occured when calling Enumrable#next on canceled call#15834
Conversation
There was a problem hiding this comment.
this looks mostly good to me, but I have a cosmetic nit:
instead of wrapping the entire read loop in a rescue clause, can we instead wrap only remote_read in, for example, a separate function that itself has the rescue CallError clause? Such a function could return nil in the case that a CallError was raised, or when remote_read itself returned nil. This would be consistent with how this sort of thing is handled in the bidi call read stream enumerable.
thanks!
There was a problem hiding this comment.
I am a little busy and am slow to review the other PR's you have sent right now, but will get to them soon.
There was a problem hiding this comment.
I think this implementation would be more efficient than your suggestion.
If the code wraps only remote_read, it calls unnecessary begin ~ rescue ~ end every remote_read.
An also becuase it has not had consistency so far, I think this code would be ok as it is. What do you think?
I am a little busy and am slow to review the other PR's you have sent right now, but will get to them soon.
Please do not worry about it 😄. but, Would you give me the write permission to this repository?
I have a plan to create other PR's. If you have little time to review, it'll be very helpful to you.
There was a problem hiding this comment.
I think this implementation would be more efficient than your suggestion.
If the code wraps only remote_read, it calls unnecessary begin ~ rescue ~ end every remote_read.
I'm not really worried about the efficiency of this method right now. And I'd much prefer to use small-scoped begin and rescue blocks rather for simplicity. If there are people complaining about the performance of this call, and there is a benchmark that shows that the begin/rescue block are causing a large penalty, then I'd be open to optimizations, but that's not the scenario currently :)
Please do not worry about it 😄. but, Would you give me the write permission to this repository?
Our repo has a policy that requires every PR to get approved, even if you have write access.
There was a problem hiding this comment.
@ganmacs this change would be good to have, but please change to reduce the scope of the begin/rescue block
There was a problem hiding this comment.
OK, I changed the scope of the being ~ rescue block.
afd133e to
c1b64e3
Compare
When
Enumrable#nextis called on already canceled call,GRPC::Cancelledshould occur instead ofGRPC::Core::CallErrorlike bidi_streamer spec and bidi_streamer does.In order to achive it,
ActiveCall#each_remote_read_then_finishsurely callreceive_and_check_statuseven ifGRPC::Core::CallErroroccurs.review please 🙏 @apolcyn