andreachild commented on code in PR #2915:
URL: https://github.com/apache/tinkerpop/pull/2915#discussion_r1862629582
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java:
##########
@@ -146,6 +146,11 @@ public synchronized
CompletableFuture<ServerGremlinExecutor> start() throws Exce
b.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK,
new WriteBufferWaterMark(settings.writeBufferLowWaterMark,
settings.writeBufferHighWaterMark));
b.childOption(ChannelOption.ALLOCATOR,
PooledByteBufAllocator.DEFAULT);
+ // Enable TCP Keep-Alive to detect if the remote peer is still
reachable.
+ // Keep-Alive sends periodic probes to check if the remote peer is
still active.
+ // If the remote peer is unreachable, the connection will be
closed, preventing resource leaks and
+ // avoiding the maintenance of stale connections.
+ b.childOption(ChannelOption.SO_KEEPALIVE, true);
Review Comment:
I agree we should protect the server against broken connections. My concern
would be that enabling the `SO_KEEPALIVE` could interfere with the idle
connection handling if it is enabled and keep connections alive which should be
considered idle and cleaned up. The `SO_KEEPALIVE` settings are OS-dependent so
it would be vary by OS when keep alive pings would kick in and how often it
would be sent.
For example assuming we fix `HttpChannelizer` to return true for `boolean
supportsIdleMonitor()` and `Settings.idleConnectionTimeout` is 30 seconds and
the OS is configured to start sending keep alive pings after 10 seconds every 1
second:
1. server receives a request and establishes a connection and sends a
response
2. client stops sending requests
3. after 10 seconds the server starts sending keep alive pings which resets
the idle connection time back to zero
4. keep alive pings are sent every second
5. after 30 seconds there is still no traffic but the connection is not
considered idle because of the keep alive pings
6. connection is not cleaned up as it is not considered idle
What do you think about one of the following solutions:
* change `SO_KEEPALIVE` to be enabled only if idle connection detection is
disabled
* change idle connection detection to be enabled by default so that broken
connections will be cleaned up as the idle timeout is reached
My preference would be the latter option as it would allow consistent
control of the timeouts at the application level instead of relying on the
varying OS-level settings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
