API to support start/stop accepting connections #1741

tkountis · 2021-08-18T00:42:42Z

Motivation:

Expose a way a user can yield accepting connection on server side, and resume on demand.?

Modifications:

ServerContext now supports an additional acceptConnections(bool) API that can be used to
hint to the server the need for start/stop accepting.

Result:

More ways to control the service on-demand.

chemicL

Minor comments and a suggestion for tests readability, but in general LGTM.

...tp-netty/src/test/java/io/servicetalk/http/netty/ConnectionAcceptingNettyHttpServerTest.java

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

...tp-netty/src/test/java/io/servicetalk/http/netty/ConnectionAcceptingNettyHttpServerTest.java

...talk-http-netty/src/main/java/io/servicetalk/http/netty/H2ServerParentConnectionContext.java

...tp-netty/src/test/java/io/servicetalk/http/netty/ConnectionAcceptingNettyHttpServerTest.java

bondolo · 2021-08-18T21:44:41Z

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

+     * tcp_max_syn_backlog</a>). These additional parameters may affect the behavior of new flows when the service
+     * is not accepting.
+     * <p>
+     * Depending on how long this stays in the {@code false} state, it may affect other timeouts (i.e., connect-timeout


It sounds like this may make the behaviour worse for clients since they might wait until connect timeout to learn that the server is not accepting connections. Is it possible to adjust the backlog queues to zero while not accepting connections and reject the queued connections as if the queue had been full?

Not being able to fully reject the inbound connection seems bad.

Good observation. That indeed may be a problem, and this is the reason I am trying to document all expectations in the javadoc so users can make an educated decision. I want to avoid making a global decision here, and give the tools to the user's to tune accordingly.

Excellent point. I was wondering whether it makes more sense to not expose such capability if we can't provide an extensive guideline how to configure the TCP settings. Wouldn't a mechanism for accepting the connection and immediately replying with a 503 HTTP error code (immediately closing the connection after the write buffer has been flushed) be more "by the book"? For gRPC that's also safe as the clients should honor that code according to this table: https://grpc.github.io/grpc/core/md_doc_http-grpc-status-mapping.html

Main problems of accepting a connections:

TLS handshake overhead. It might produce significant CPU usage, affecting other requests.

There is no guarantee that a new attempt to connect supports/expects HTTP protocol or will send a request we can respond to.

Therefore, this mechanism is introduced to stop as soon as possible.

Motivation: Expose a way a user can yield accepting connection on server side, and resume on demand.? Modifications: ServerContext now supports an additional `acceptConnections(bool)` API that can be used to hint to the server the need for start/stop accepting. Result: More ways to control the service on-demand.

tkountis · 2021-08-19T15:39:16Z

Build failure due: #1579

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

...etalk-transport-api/src/main/java/io/servicetalk/transport/api/ServiceTalkSocketOptions.java

...talk-http-netty/src/main/java/io/servicetalk/http/netty/H2ServerParentConnectionContext.java

...tp-netty/src/test/java/io/servicetalk/http/netty/ConnectionAcceptingNettyHttpServerTest.java

idelpivnitskiy

LGTM after the patch is applied

idelpivnitskiy · 2021-08-25T23:18:09Z

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

-     * SYN backlog (i.e., in case of Linux <a href="https://www.kernel.org/doc/Documentation/networking/ip-sysctl.txt">
-     * tcp_max_syn_backlog</a>). These additional parameters may affect the behavior of new flows when the service
-     * is not accepting.
+     * SYN backlog (in the case of linux). This can be tuned:


Consider clarifying that it can be tuned at OS level or per server instance using ServiceTalkSocketOptions#SO_BACKLOG option.

I know that you don't have access to HTTP classes from here, but maybe even without referencing an exact method, consider clarifying that they need to use listenSocketOption on the builder, not a socketOption method. It might be a useful hint.

This is not tunable on the per server instance. so_backlog = accept_queue. SYN backlog is a different setting, only at the OS level. I am not elaborating further on it, because its very OS specific.

Thanks for clarifying!

...talk-tcp-netty-internal/src/main/java/io/servicetalk/tcp/netty/internal/TcpServerBinder.java

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

tkountis · 2021-08-26T10:25:11Z

Another failure due to #1579

idelpivnitskiy

🚀

tkountis force-pushed the capacity/stop-accepting branch from d2ac0b3 to cb20348 Compare August 18, 2021 00:45

tkountis requested a review from idelpivnitskiy August 18, 2021 00:48

tkountis force-pushed the capacity/stop-accepting branch from cb20348 to e07e22c Compare August 18, 2021 09:29

chemicL approved these changes Aug 18, 2021

View reviewed changes

tkountis force-pushed the capacity/stop-accepting branch 2 times, most recently from 8235edf to e558089 Compare August 18, 2021 14:57

tkountis commented Aug 18, 2021

View reviewed changes

...tp-netty/src/test/java/io/servicetalk/http/netty/ConnectionAcceptingNettyHttpServerTest.java Show resolved Hide resolved

tkountis force-pushed the capacity/stop-accepting branch from e558089 to 620f978 Compare August 18, 2021 15:05

bondolo reviewed Aug 18, 2021

View reviewed changes

tkountis force-pushed the capacity/stop-accepting branch from 620f978 to 673526a Compare August 19, 2021 13:46

tkountis force-pushed the capacity/stop-accepting branch from 673526a to 95921f8 Compare August 19, 2021 13:49

tkountis self-assigned this Aug 19, 2021

idelpivnitskiy requested changes Aug 20, 2021

View reviewed changes

Comments

ca60a1c

idelpivnitskiy reviewed Aug 26, 2021

View reviewed changes

tkountis mentioned this pull request Aug 26, 2021

Transport observer should be aware of accepted inactive channels #1755

Open

Comments

981ee6d

idelpivnitskiy approved these changes Aug 26, 2021

View reviewed changes

tkountis merged commit 6beebe8 into apple:main Aug 26, 2021

tkountis deleted the capacity/stop-accepting branch August 26, 2021 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API to support start/stop accepting connections #1741

API to support start/stop accepting connections #1741

tkountis commented Aug 18, 2021

chemicL left a comment

bondolo Aug 18, 2021

tkountis Aug 19, 2021

chemicL Aug 19, 2021 •

edited

Loading

idelpivnitskiy Aug 25, 2021 •

edited

Loading

tkountis commented Aug 19, 2021

idelpivnitskiy left a comment

idelpivnitskiy Aug 25, 2021

tkountis Aug 26, 2021 •

edited

Loading

idelpivnitskiy Aug 26, 2021

tkountis commented Aug 26, 2021

idelpivnitskiy left a comment

API to support start/stop accepting connections #1741

API to support start/stop accepting connections #1741

Conversation

tkountis commented Aug 18, 2021

chemicL left a comment

Choose a reason for hiding this comment

bondolo Aug 18, 2021

Choose a reason for hiding this comment

tkountis Aug 19, 2021

Choose a reason for hiding this comment

chemicL Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

idelpivnitskiy Aug 25, 2021 • edited Loading

Choose a reason for hiding this comment

tkountis commented Aug 19, 2021

idelpivnitskiy left a comment

Choose a reason for hiding this comment

idelpivnitskiy Aug 25, 2021

Choose a reason for hiding this comment

tkountis Aug 26, 2021 • edited Loading

Choose a reason for hiding this comment

idelpivnitskiy Aug 26, 2021

Choose a reason for hiding this comment

tkountis commented Aug 26, 2021

idelpivnitskiy left a comment

Choose a reason for hiding this comment

chemicL Aug 19, 2021 •

edited

Loading

idelpivnitskiy Aug 25, 2021 •

edited

Loading

tkountis Aug 26, 2021 •

edited

Loading