Expose `SocketOption` querying on `ConnectionContext` API #950

idelpivnitskiy · 2020-02-27T21:42:09Z

Motivation:

Users should be able to read socket options for created connections.

Modifications:

Add ConnectionContext.socketOption API to query socket options;
Add ConnectionContextSocketOptionTest to test new API;

Results:

Users can read socket options of the connections.

idelpivnitskiy · 2020-02-27T21:46:09Z

...lk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionContextSocketOptionTest.java

+    public void tcpStandardSocketOptionIsNotNull() throws Exception {
+        testSocketOption("TCP_NODELAY", Boolean.class, is(notNullValue()),
+                // HTTP_2 stream channel does not have TCP_NODELAY option
+                protocol == Protocol.HTTP_2 ? equalTo("null") : not(equalTo("null")));


Server connection context uses child-stream for h2 that does not allow users to query multiple TCP options for the original parent connection, like TCP_NODELAY / SO_RCVBUF / SO_SNDBUF / etc.
Do we want to keep it as-is? We can expose options for the original TCP connection by keeping ref for the parent's ChannelConfig.

Are there any options for H2 that are applicable for a stream? If not, should we always just query the options from the parent context?

From what I see in netty, it creates a new DefaultChannelConfig for each stream. And these config is a fresh new and is not related to the parent's ChannelConfig. Therefore, each channel has its own subset of options. In theory (but not in the current ST) some code may modify that options for each stream independently (like AUTO_READ, AUTO_CLOSE`, etc.) and it will not have side-effects for parent channel.

I think it will be more practical for users to be interested in parent channel options. It will require one more reference in DefaultNettyConnection. Let me try and will see the overhead.

Done: 4fbb1f9

idelpivnitskiy · 2020-02-27T22:18:28Z

JDK11 build failed due to docker issue

@servicetalk-bot test this please

idelpivnitskiy · 2020-02-28T22:12:11Z

One of the builds failed with #952

...-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/SocketOptionUtils.java

servicetalk-transport-api/src/main/java/io/servicetalk/transport/api/ConnectionContext.java

NiteshKant · 2020-02-28T19:41:37Z

...-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/SocketOptionUtils.java

+        if (option == ServiceTalkSocketOptions.IDLE_TIMEOUT) {
+            return idleTimeoutMs == null ? null : (T) idleTimeoutMs;    // TODO: return (T) idleTimeoutMs;
+        }
+        // Try to look for a ChannelOption with the same name and type:


In the method above we do not set any option that we do not recognize in the channel config then is trying to lookup for an unknown value here reasonable? Can we be consistent between get and set?

Netty has a lot more other options that users might be interested in to query, especially for different transports, like TCP_FASTOPEN_CONNECT for epoll. See EpollChannelOption class from netty.

If we will follow the policy to be consistent between get and set, every time users request a new option for querying we will need to provide one more ServiceTalkSocketOption constant and implement support for setting this option. However, users may not bother with setting it, just querying. Also, maintaining a single or separate lists of ServiceTalkSocketOption constants that are applicable only for a specific transport may be not ideal.

SocketOption<T> is a public JDK API. Users can use it to create the required options for querying without asking us. The only risk here is that they can take internal ByteBufAllocator from netty. But that will be their risky choice or we can prevent specific options from querying. The list is short and contains only 3 options that are dengerous: ALLOCATOR, RCVBUF_ALLOCATOR, MESSAGE_SIZE_ESTIMATOR. All other netty options are read-only objects.

Considering that overhead of keeping get and set in consistency for every new option, I decided to let users query what they want with existing API.

As we discussed offline, I made get/set consistent in 09d0918.
Here is an issue to enhance these two methods to support custom options: #953

...-netty-internal/src/main/java/io/servicetalk/transport/netty/internal/SocketOptionUtils.java

NiteshKant · 2020-02-28T22:06:05Z

...y-internal/src/main/java/io/servicetalk/transport/netty/internal/DefaultNettyConnection.java

    }

    private DefaultNettyConnection(Channel channel, BufferAllocator allocator, Executor executor,
                                   TerminalPredicate<Read> terminalMsgPredicate, CloseHandler closeHandler,
-                                   FlushStrategy flushStrategy, ExecutionStrategy executionStrategy,
-                                   @Nullable SSLSession sslSession) {
+                                   FlushStrategy flushStrategy, @Nullable Long idleTimeoutMs,


I am somewhat apprehensive about individually adding options here which are otherwise not stored by netty. Anything else we add will mean we add more arguments here.

Can we instead take+store the TcpConfig here and pass the same to SocketOptionUtils for querying? If we do use TcpConfig we can use that to also get the FlushStrategy

Thought about that but decided to defer until we add anything else to better understand how to structure it (maybe it will be better to have an object to keep these options instead of using the full TcpConfig).

We do not expose AbstractReadOnlyTcpConfig as a public class for now. If you think we can make AbstractReadOnlyTcpConfig public (it lives in tcp-netty-internal module) to keep DefaultNettyConnection reusable for client and server side, I can do the change now. Otherwise, let's defer?

We may also consider using a single ExecutionContext instead of other 3 separate arguments: BufferAllocator, Executor, ExecutionStrategy. Can do here or as a follow-up.

Can we just use TcpConfig for now as these are internal classes and we can make sure we do not update config? If you want, we can also wrap the config to throw from the setters.

I am also OK to defer, your call.

Will defer that, thanks

NiteshKant · 2020-02-28T22:09:15Z

...lk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionContextSocketOptionTest.java

+    public void tcpStandardSocketOptionIsNotNull() throws Exception {
+        testSocketOption("TCP_NODELAY", Boolean.class, is(notNullValue()),
+                // HTTP_2 stream channel does not have TCP_NODELAY option
+                protocol == Protocol.HTTP_2 ? equalTo("null") : not(equalTo("null")));


Are there any options for H2 that are applicable for a stream? If not, should we always just query the options from the parent context?

...lk-http-netty/src/test/java/io/servicetalk/http/netty/ConnectionContextSocketOptionTest.java

idelpivnitskiy added 10 commits February 27, 2020 13:40

Expose access to query socket options on ConnectionContext API

3d5deda

Try to look for a ChannelOption with the same name and type

31474f1

use long

e3b9292

Keep idleTimeoutMs value in the ConnectionContext

9a796e5

idleTimeoutMs: long -> Long

03aa733

Add SocketOptionUtils

9f9a470

Change name for ServiceTalkSocketOptions.CONNECT_TIMEOUT

cbcf08b

ConnectionContextSocketOptionTest

376736f

Improve tests

0ef924f

Update copyright years

3ae4089

idelpivnitskiy requested review from NiteshKant and Scottmitch February 27, 2020 21:42

idelpivnitskiy commented Feb 27, 2020

View reviewed changes

idelpivnitskiy added 5 commits February 27, 2020 17:43

Fix ClassCastException in JDK11

464a55d

small fixes for tests

3b0f96f

address todo

1de8c60

add supportedSocketOptionWithIncorrectTypeIsNull test

fb1acbb

Merge remote-tracking branch 'upstream/master' into socket-options

f1336ad

NiteshKant suggested changes Feb 28, 2020

View reviewed changes

idelpivnitskiy added 4 commits February 28, 2020 15:51

Address review comments

7adf85d

Remove unused logger

e288425

simplify test assertions

d4e4103

Keep parentChannelConfig in DefaultNettyConnection

4fbb1f9

idelpivnitskiy requested a review from NiteshKant February 29, 2020 00:26

idelpivnitskiy added 4 commits March 2, 2020 17:47

small fixes for tests

b53b093

Add type for SocketOptionUtils.addOption

6302a06

Throw if users provide an unknown or unsupported SocketOption

09d0918

Merge remote-tracking branch 'upstream/master' into socket-options

7a87203

NiteshKant approved these changes Mar 3, 2020

View reviewed changes

idelpivnitskiy merged commit 592a0c1 into apple:master Mar 3, 2020

idelpivnitskiy deleted the socket-options branch March 3, 2020 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose `SocketOption` querying on `ConnectionContext` API #950

Expose `SocketOption` querying on `ConnectionContext` API #950

idelpivnitskiy commented Feb 27, 2020 •

edited

Loading

idelpivnitskiy Feb 27, 2020

NiteshKant Feb 28, 2020

idelpivnitskiy Feb 28, 2020 •

edited

Loading

idelpivnitskiy Feb 29, 2020

idelpivnitskiy commented Feb 27, 2020

idelpivnitskiy commented Feb 28, 2020

NiteshKant Feb 28, 2020

idelpivnitskiy Feb 28, 2020 •

edited

Loading

idelpivnitskiy Mar 3, 2020

NiteshKant Feb 28, 2020

idelpivnitskiy Feb 28, 2020 •

edited

Loading

NiteshKant Feb 29, 2020

idelpivnitskiy Mar 3, 2020

NiteshKant Feb 28, 2020

Expose SocketOption querying on ConnectionContext API #950

Expose SocketOption querying on ConnectionContext API #950

Conversation

idelpivnitskiy commented Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy commented Feb 27, 2020

idelpivnitskiy commented Feb 28, 2020

Choose a reason for hiding this comment

idelpivnitskiy Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idelpivnitskiy Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Expose `SocketOption` querying on `ConnectionContext` API #950

Expose `SocketOption` querying on `ConnectionContext` API #950

idelpivnitskiy commented Feb 27, 2020 •

edited

Loading

idelpivnitskiy Feb 28, 2020 •

edited

Loading

idelpivnitskiy Feb 28, 2020 •

edited

Loading

idelpivnitskiy Feb 28, 2020 •

edited

Loading