Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid the issue of connections not being released. #2915

Merged
merged 1 commit into from
Jan 7, 2025
Merged

Avoid the issue of connections not being released. #2915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ image::https://raw.githubusercontent.com/apache/tinkerpop/master/docs/static/ima
* Use `TraversalStrategyProxy` in Java, or `TraversalStrategy` in Python to pass custom strategies in traversals
* Removed `minSize` setting for Gremlin Driver connection pool since connections are now short-lived HTTP connections
* Added `idleConnectionTimeout` setting for Gremlin Driver and automatic closing of idle connections
* Enable TCP Keep-Alive in GremlinServer.

== TinkerPop 3.7.0 (Gremfir Master of the Pan Flute)

Expand Down
5 changes: 5 additions & 0 deletions gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@andreachild andreachild Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @dh-cloud, thanks for noticing this issue with potential resource leaks in the gremlin-server.

In the master branch we have switched to HTTP (away from web sockets in 3.7) which means there will be much more short term connections compared to web sockets which uses long lived connections. When there is decreased activity we should aim to release the resources and thus not use the keep alive option to keep the connection active.

There is still an issue with potential resource leak but instead we can fix this using the existing idleConnectionTimeout setting. The issue here is that during the switch from web sockets to HTTP, we neglected to enable idle connection monitoring for HTTP 😢 . What do you think about changing this PR to enable idle connection monitoring instead?

See below for references in the code as to what I'm referring to:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andreachild
Yes, I agree that we can also add handling for idle connections, but the original intention of this PR was to destroy broken connections, especially in scenarios like the following: when a client establishes a long connection with the gremlin-server, and then the client's network card is suddenly unplugged to simulate a client-side network issue. In this case, when the server uses netstat -antp to check, the connection still appears to be established, but using the ping command to test the client's address fails, and this connection will never be destroyed. I have reproduced this scenario.

Feel free to let me know if you need any further adjustments!

Copy link
Contributor

@andreachild andreachild Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have fully understood your point. For the requirement to disconnect abnormal connections, I believe we can consider it from two dimensions:

When boolean supportsIdleMonitor() is false, i.e., idle connection detection is disabled, we can enable SO_KEEPALIVE as a fallback to prevent broken connections from remaining occupied.
When supportsIdleMonitor() is true, we need to improve the handling logic for idle timeout connections.
In the TinkerPop master source code, I noticed that settings.idleConnectionTimeout and settings.keepAliveInterval are both default to 0. As a result, the new IdleStateHandler in AbstractChannelizer.class will not have any effect.

if (supportsIdleMonitor()) {
    final int idleConnectionTimeout = (int) (settings.idleConnectionTimeout / 1000);
    final int keepAliveInterval = (int) (settings.keepAliveInterval / 1000);
    pipeline.addLast(new IdleStateHandler(idleConnectionTimeout, keepAliveInterval, 0));
}

Setting specific values for idleConnectionTimeout and keepAliveInterval is a complex task and has significant impact, as they control the idle threshold for idle connections. However, different use cases have different requirements, and it is challenging to provide a one-size-fits-all recommendation.

For this PR, I force pushed new commit implementing the fallback measure only when boolean supportsIdleMonitor() is false.

I hope this helps! If you need any further adjustments, feel free to let me know.


// fire off any lifecycle scripts that were provided by the user. hooks get initialized during
// ServerGremlinExecutor initialization
Expand Down
Loading