Skip to content
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

Fix concurrent modification error in GrpcWebClientChannel.terminate #332

Merged
merged 11 commits into from
Nov 12, 2020

Conversation

isaldana
Copy link
Contributor

Fixes #331

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 24, 2020

CLA Check
The committers are authorized under a signed CLA.

@mit-mit
Copy link
Collaborator

mit-mit commented Sep 17, 2020

cc @mraleph

@mit-mit
Copy link
Collaborator

mit-mit commented Sep 17, 2020

@isaldana you mentioned in #306 (comment) that this might also fix #206 ?

@isaldana
Copy link
Contributor Author

@mit-mit I think so. The cancel() call eventually calls terminate() on the stream as show here https://github.com/grpc/grpc-dart/blob/master/lib/src/client/call.dart#L328. So this addresses the issue with terminate() on web.

Copy link
Member

@mraleph mraleph left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution and sorry for the delay with reviews.

for (XhrTransportStream request in _requests) {
request.terminate();
while (_requests.isNotEmpty) {
final XhrTransportStream request = _requests.first;
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest:

while (_requests.isNotEmpty) {
  final requests = List.of(_requests);
  _requests.clear();

    
  await Future.wait(requests.map((rq) => rq.terminate()));
}

this would terminate() all requests in parallel rather then sequentially.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mraleph AFAIK, there is no need to wait for the requests to terminate. It was just the nature of the loop since I didn't want to copy the requests list. I made the change to copy the list of requests and call terminate on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mraleph Please hold on before merging this. I've been running https://github.com/isaldana/grpc-dart for a while and I just realized that I forgot to include isaldana@0d23114 in this PR.

@mraleph
Copy link
Member

mraleph commented Oct 28, 2020

@isaldana is this in a mergable? Could you rebase and add a test. We now have a gRPC-web testing harness you could use.

Copy link
Member

@mraleph mraleph left a comment

Choose a reason for hiding this comment

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

I have added test and going to merge.

@mraleph mraleph changed the title Allow terminate() to get called on a web client connection Fix concurrent modification error in GrpcWebClientChannel.terminate Nov 12, 2020
@mraleph mraleph merged commit 275cc54 into grpc:master Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when terminating web connections.
3 participants