Remove channel.awaitTermination() from S2A ChannelResource.

[rmehta19]

b/388769143

[rmehta19]

@ejona86 , would you be able to review this PR? I sent you an email with some additional context. Thank you.

[ejona86]

You can and should link to bugs here, so you'd include a reference to b/388769143.

[ejona86]

Why move away from shutdownNow()? Was that important? I see b/388769143#comment45, but it is unclear if you tried "remove awaitTermination, keep shutdownNow()".

The requirement to call close() is this channel no longer has an RPCs on it. So 1) shutdown() and shutdownNow() should behave the same (and we use shutdownNow() to make it more obvious if something is broken) and 2) awaitTermination() would be expected to be speedy.

I could understand removing the awaitTermination(), as it doesn't actually do much. Considering how S2A is used on the transport thread, maybe awaitTermination() could deadlock, as it looks like SharedResourceHolder holds a lock when close() is called, and that same lock is needed when creating the instance. But with the S2A channel using its own network thread, I don't see an actual construction that would deadlock, but it is conceivable.

It might be a good idea to discuss this over a meeting. While this change may fix things, I'm a bit worried there is something more fundamental that still remains, and this is just working around the real issue.

[rmehta19]

Why move away from shutdownNow()? Was that important? I see b/388769143#comment45, but it is unclear if you tried "remove awaitTermination, keep shutdownNow()".

Thanks for calling this out and for providing context on SharedResourceHolder. I did not try "remove awaitTermination, keep shutdownNow()". However, I did try that today, and it does also resolve the issue. So it looks like there is no need to change shutdownNow() to shutdown(). So to be concrete, two options resolve the issue we are seeing:

public void close(Channel instanceChannel) {
      checkNotNull(instanceChannel);
      ManagedChannel channel = (ManagedChannel) instanceChannel;
      channel.shutdownNow();
    }

and

// current PR commit.
public void close(Channel instanceChannel) {
      checkNotNull(instanceChannel);
      ManagedChannel channel = (ManagedChannel) instanceChannel;
      channel.shutdown();
    }

The requirement to call close() is this channel no longer has an RPCs on it. So 1) shutdown() and shutdownNow() should behave the same (and we use shutdownNow() to make it more obvious if something is broken) and 2) awaitTermination() would be expected to be speedy.

I think the test I ran today (per your suggestion: "remove awaitTermination, keep shutdownNow()") confirms (1).

I added a meeting to discuss this further. We can discuss this PR and the issue in b/388769143#comment45 further then.

[ejona86]

It sounds like there's just one option: "remove awaitTermination()". The other changes don't seem relevant. Sure, you could swap to shutdown(), but you could also add some code to calculate pi. But that has no impact to the problem at hand.