Reconnect disconnected channels automatically by kpayson64 · Pull Request #11154 · grpc/grpc

kpayson64 · 2017-05-16T17:33:51Z

Submission pending on #10813

The memory leak described in that poll request is likely the cause of #9461,
and without a fix this will introduce that leak to all channels.

nathanielmanistaatgoogle

Just to double-check, because I think it might be slightly amazing: you've confirmed that the test fails in the absence of the non-test portion of this change?

nathanielmanistaatgoogle · 2017-05-16T17:37:30Z

This should be a TODO (possibly a TODO(https://github.com/grpc/grpc/issues/11043)?).

I think we can close out #11043 after this change.
#9884 is the "Make a proper fix in c-core"

I don't think this change is sufficient to close #11043 because #11043 is a wrapped-languages issue and this is a Python-only change.

#9884 might be the right issue for scoping the temporary-ness of this workaround; I think I'm a little thrown by the way it's being called "retries" and being described as a feature when I keep encountering it as "not trying hard enough the first time" and thinking of it as a bug.

nathanielmanistaatgoogle · 2017-05-16T17:38:48Z

How certain are you that this is necessary? What does this do that the _moot call on the next line doesn't also do? If we can eliminate this call, we can also eliminate the maintenance of the dummy callback as an instance field, right?

Removed, thanks.

nathanielmanistaatgoogle · 2017-05-16T17:40:41Z

Just for test sanity's sake let's make the value of this constant different from the value of _REQUEST.

nathanielmanistaatgoogle · 2017-05-16T17:41:19Z

Always leave the unnecessary ess off the end of assertEqual.

nathanielmanistaatgoogle · 2017-05-16T17:50:57Z

"Rebalance" the statements in this test so that nothing in the test method "undoes" what was done in setUp and also so that nothing in the test method "does" what will later be undone in tearDown. (I think this will mean that what will be left in your test will be only instantiating the thread pool, at which point you may choose to simply not bother with having a separate setUp/tearDown.)

A unit test with a setUp/tearDown and also only a single test method is particularly susceptible to premature generalization.

Removed setUp/tearDown

nathanielmanistaatgoogle · 2017-05-16T17:51:42Z

We haven't been good about this in the past but it's on my list of things to clean up (and in the meantime, to avoid digging deeper): use the PEP8-style name test_reconnect.

nathanielmanistaatgoogle · 2017-05-16T17:52:07Z

Missing leading underscore on this name.

kpayson64 · 2017-05-16T18:18:01Z

When the non-test changes are removed, the test fails:

  File "src/python/grpcio_tests/tests/unit/_reconnect_test.py", line 70, in testReconnect
    self.assertEquals(_RESPONSE, multi_callable(_REQUEST))
  File "/usr/local/google/home/kpayson/grpc2/py27/local/lib/python2.7/site-packages/grpc/_channel.py", line 507, in __call__
    return _end_unary_response_blocking(state, call, False, deadline)
  File "/usr/local/google/home/kpayson/grpc2/py27/local/lib/python2.7/site-packages/grpc/_channel.py", line 455, in _end_unary_response_blocking
    raise _Rendezvous(state, None, None, deadline)
_Rendezvous: <_Rendezvous of RPC that terminated with (StatusCode.UNAVAILABLE, Endpoint read failed)>

kpayson64 · 2017-05-16T18:51:31Z

@nathanielmanistaatgoogle Addressed comments, back to you

nathanielmanistaatgoogle · 2017-05-16T18:52:54Z

What's the significance of returning None, None rather than just None?

The second none is the try_to_connect arg, not part of the lambda.

nathanielmanistaatgoogle · 2017-05-16T18:55:23Z

Typography nitpick: missing colon after closing parenthesis and start the prose of the comment on the same line rather than line-wrapping after the TODO(...):.

Huh. I guess the colon isn't required. Aww phooey; I've always liked it.

nathanielmanistaatgoogle · 2017-05-16T19:03:47Z

I don't think this change is sufficient to close #11043 because #11043 is a wrapped-languages issue and this is a Python-only change.

#9884 might be the right issue for scoping the temporary-ness of this workaround; I think I'm a little thrown by the way it's being called "retries" and being described as a feature when I keep encountering it as "not trying hard enough the first time" and thinking of it as a bug.

nathanielmanistaatgoogle

I'm a little skittish about spawning a thread per channel, but... okay.

nathanielmanistaatgoogle · 2017-05-16T19:05:42Z

assertEqual without the trailing ess here too.

kpayson64 added disposition/DO NOT MERGE lang/Python labels May 16, 2017

kpayson64 assigned nathanielmanistaatgoogle May 16, 2017

kpayson64 requested a review from nathanielmanistaatgoogle May 16, 2017 17:33

googlebot added the cla: yes label May 16, 2017

nathanielmanistaatgoogle suggested changes May 16, 2017

View reviewed changes

kpayson64 force-pushed the hack_for_disconnects branch from 1b471b7 to 30d066b Compare May 16, 2017 18:50

nathanielmanistaatgoogle reviewed May 16, 2017

View reviewed changes

nathanielmanistaatgoogle approved these changes May 16, 2017

View reviewed changes

kpayson64 changed the base branch from master to v1.3.x May 19, 2017 21:31

kpayson64 force-pushed the hack_for_disconnects branch 2 times, most recently from 42f53b9 to 5bcbc79 Compare May 19, 2017 23:44

Reconnect disconnected channels automatically

8f7bc54

kpayson64 force-pushed the hack_for_disconnects branch from 5bcbc79 to 8f7bc54 Compare May 22, 2017 17:07

kpayson64 removed the disposition/DO NOT MERGE label May 22, 2017

kpayson64 merged commit 866190f into grpc:v1.3.x May 22, 2017

nathanielmanistaatgoogle mentioned this pull request May 31, 2017

v0.25.0 with grpcio==1.3.5 hangs in completion_queue.poll() googleapis/google-cloud-python#3450

Closed

This was referenced Jun 8, 2017

Idle channels sometimes fails RPCs with UNAVAILABLE status #11043

Closed

Channel doesn't attempt to reconnect after idle time #8538

Closed

Secure read failed. Cloud logging error #9217

Closed

kpayson64 mentioned this pull request Dec 11, 2017

Python Client 1.0.0 suffers connection unavailable after service restarted #8296

Closed

lock Bot locked as resolved and limited conversation to collaborators Jan 23, 2019

Conversation

kpayson64 commented May 16, 2017

Uh oh!

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kpayson64 commented May 16, 2017

Uh oh!

kpayson64 commented May 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanielmanistaatgoogle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants