Skip to content

GRPC::Cancelled should be occured when calling Enumrable#next on canceled call#15834

Merged
apolcyn merged 1 commit intogrpc:masterfrom
ganmacs:ensure-that-cancelled-server_streamer-call-raise-cancell
Jun 22, 2018
Merged

GRPC::Cancelled should be occured when calling Enumrable#next on canceled call#15834
apolcyn merged 1 commit intogrpc:masterfrom
ganmacs:ensure-that-cancelled-server_streamer-call-raise-cancell

Conversation

@ganmacs
Copy link
Copy Markdown
Contributor

@ganmacs ganmacs commented Jun 21, 2018

When Enumrable#next is called on already canceled call, GRPC::Cancelled should occur instead of GRPC::Core::CallError like bidi_streamer spec and bidi_streamer does.
In order to achive it, ActiveCall#each_remote_read_then_finish surely call receive_and_check_status even if GRPC::Core::CallError occurs.

review please 🙏 @apolcyn

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ganmacs this change would be good to have, but please change to reduce the scope of the begin/rescue block

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I changed the scope of the being ~ rescue block.

@ganmacs ganmacs force-pushed the ensure-that-cancelled-server_streamer-call-raise-cancell branch from afd133e to c1b64e3 Compare June 22, 2018 19:19
@apolcyn apolcyn merged commit 24416c7 into grpc:master Jun 22, 2018
@ganmacs ganmacs deleted the ensure-that-cancelled-server_streamer-call-raise-cancell branch June 23, 2018 02:38
@apolcyn apolcyn added the release notes: yes Indicates if PR needs to be in release notes label Jul 20, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Oct 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

lang/ruby release notes: yes Indicates if PR needs to be in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants