ALTS: release handshaker channel if no longer needed #5210

jiangtaoli2016 · 2019-01-04T00:54:23Z

In ALTS and Google Default Channel, we create a shared channel to handshaker service lazily. This PR releases the channel when ProtocolNegotiator close() is called.

jiangtaoli2016 · 2019-01-04T21:46:05Z

#4755

carl-mastrangelo · 2019-01-04T23:31:30Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

+      final ObjectPool<Channel> handshakerChannelPool) {
    final class ServerAltsProtocolNegotiator extends AltsProtocolNegotiator {
+
+      private Channel handshakerChannel = null;


s/ = null;/;/

it's implicit in java.

carl-mastrangelo · 2019-01-04T23:33:30Z

alts/src/main/java/io/grpc/alts/internal/TsiHandshakerFactory.java


  /** Creates a new handshaker. */
-  TsiHandshaker newHandshaker(@Nullable String authority);
+  TsiHandshaker newHandshaker(@Nullable Channel handshakerChannel, @Nullable String authority);


I fear this may have internal repercussions.

It is not an issue. I have done it before. Let me prepare an import patch.

This looks wrong. The reason authority is passed is because it could be different every time. This channel is also specific to one handshaker implementation.

Instead, you should pass some object when creating the TsiHandshakerFactory. Problably an ObjectPool.

srini100 · 2019-01-07T18:14:28Z

/cc

ejona86 · 2019-01-08T17:06:33Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

      public void close() {
        logger.finest("ALTS Client ProtocolNegotiator Closed");
-        // TODO(jiangtaoli2016): release resources
+        handshakerChannelPool.returnObject(handshakerChannel);


This could release the object before all the Handlers have finished using it.

If we don't return the channel here, where shall we return the channel?

I was thinking we would return it here and in the handshakers.

However, I checked and this close is called on termination, which means it is guaranteed no handshakes are still occurring. So there's nothing needed to be changed here.

ejona86 · 2019-01-08T17:07:43Z

alts/src/main/java/io/grpc/alts/internal/TsiHandshakerFactory.java


  /** Creates a new handshaker. */
-  TsiHandshaker newHandshaker(@Nullable String authority);
+  TsiHandshaker newHandshaker(@Nullable Channel handshakerChannel, @Nullable String authority);


This looks wrong. The reason authority is passed is because it could be different every time. This channel is also specific to one handshaker implementation.

Instead, you should pass some object when creating the TsiHandshakerFactory. Problably an ObjectPool.

ejona86 · 2019-01-08T21:03:54Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

      public void close() {
        logger.finest("ALTS Client ProtocolNegotiator Closed");
-        // TODO(jiangtaoli2016): release resources
+        handshakerChannelPool.returnObject(handshakerChannel);


I was thinking we would return it here and in the handshakers.

However, I checked and this close is called on termination, which means it is guaranteed no handshakes are still occurring. So there's nothing needed to be changed here.

ejona86 · 2019-01-08T21:04:06Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

      public Handler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
-        TsiHandshaker handshaker = handshakerFactory.newHandshaker(grpcHandler.getAuthority());
+        if (handshakerChannel == null) {
+          handshakerChannel = handshakerChannelPool.getObject();


This is not thread-safe.

ejona86 · 2019-01-11T19:10:04Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

      public void close() {
        logger.finest("ALTS Server ProtocolNegotiator Closed");
-        // TODO(jiangtaoli2016): release resources
+        handshakerChannelPool.returnObject(lazyHandshakerChannel.get());


This creates it if it wasn't used. If that's being done there's no point in creating it lazily.

Add a close/shutdown/release to LazyChannel? It would be a synchronized method and release the object only if it was created. That also means you wouldn't need to pass ObjectPool to AltsProtocolNegotiator which assumed it is the same instance as LazyChannel is using.

Good suggestion! done.

ejona86 · 2019-01-11T19:16:36Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java


+  /** Channel created from a channel pool lazily. */
+  public static class LazyChannel {
+    private ObjectPool<Channel> channelPool;


jiangtaoli2016

code revised.

jiangtaoli2016 · 2019-01-11T21:00:31Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

      public void close() {
        logger.finest("ALTS Server ProtocolNegotiator Closed");
-        // TODO(jiangtaoli2016): release resources
+        handshakerChannelPool.returnObject(lazyHandshakerChannel.get());


Good suggestion! done.

jiangtaoli2016 · 2019-01-11T21:00:39Z

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java


+  /** Channel created from a channel pool lazily. */
+  public static class LazyChannel {
+    private ObjectPool<Channel> channelPool;


jiangtaoli2016 · 2019-01-11T22:56:40Z

Thanks Eric and Carl for helpful discussion and suggestions! It is much nicer than the original implementation.

ALTS: release handshaker channel if no longer needed

bd374b7

jiangtaoli2016 requested a review from carl-mastrangelo January 4, 2019 00:54

jiangtaoli2016 added 2 commits January 3, 2019 17:55

Merge branch 'master' into release_handshaker_channel

ccbd5ff

Add comments to TsiHandshakerFactory

efce2df

jiangtaoli2016 added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Jan 4, 2019

kokoro-team removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Jan 4, 2019

carl-mastrangelo reviewed Jan 4, 2019

View reviewed changes

Revise based on review comments

e618040

ejona86 requested changes Jan 8, 2019

View reviewed changes

ejona86 reviewed Jan 8, 2019

View reviewed changes

jiangtaoli2016 added 2 commits January 11, 2019 10:27

Use LazyChannel insted of modifying TsiHandshakerFactory interface

9b9aa01

Merge branch 'master' into release_handshaker_channel

8077cba

ejona86 reviewed Jan 11, 2019

View reviewed changes

jiangtaoli2016 commented Jan 11, 2019

View reviewed changes

Add a close to LazyChannel

5fbd5ed

jiangtaoli2016 force-pushed the release_handshaker_channel branch from 65013af to 5fbd5ed Compare January 11, 2019 21:06

ejona86 approved these changes Jan 11, 2019

View reviewed changes

jiangtaoli2016 merged commit 4d90b37 into grpc:master Jan 11, 2019

srini100 mentioned this pull request Jan 22, 2019

ALTS: release handshaker service channel if it is not used. #4755

Closed

mziccard mentioned this pull request Jan 23, 2019

Use GoogleDefaultChannelBuilder googleapis/gax-java#627

Merged

lock bot locked as resolved and limited conversation to collaborators Apr 11, 2019

ALTS: release handshaker channel if no longer needed #5210

ALTS: release handshaker channel if no longer needed #5210

Uh oh!

Conversation

jiangtaoli2016 commented Jan 4, 2019

Uh oh!

jiangtaoli2016 commented Jan 4, 2019

Uh oh!

carl-mastrangelo Jan 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

srini100 commented Jan 7, 2019

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!

jiangtaoli2016 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!

jiangtaoli2016 commented Jan 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

carl-mastrangelo Jan 4, 2019 •

edited

Loading