Skip to content

SNOW-3010074 Fix incorrect closing of HTTP connections#2481

Open
sfc-gh-pfus wants to merge 1 commit intomasterfrom
SNOW-3010074-fin-wait-2
Open

SNOW-3010074 Fix incorrect closing of HTTP connections#2481
sfc-gh-pfus wants to merge 1 commit intomasterfrom
SNOW-3010074-fin-wait-2

Conversation

@sfc-gh-pfus
Copy link
Collaborator

@sfc-gh-pfus sfc-gh-pfus commented Feb 6, 2026

Overview

SNOW-3010074 Fixes a couple of problems related to HTTP connections closing.

  1. Run a background thread, that monitors idle or expired connections and close them by setting evictIdleConnections and evictExpiredConnections. This one creates a thread that actively monitors such connections. This thread is closed on either JVM shutdown (daemon) or when HTTP client is explicitly closed.
  2. Close HTTP client correctly. We cache HTTP clients, but in case of specific exceptions, we remove a connection from the cache. Added the code that closes HTTP client then, to prevent thread leaks.
  3. Use setValidateAfterInactivity on a pool. With this setting, before retrieving a connection from a pool, the pool checks if the connection is still valid (i.e. it is not closed on the remote end).
  4. Introduced net.snowflake.jdbc.idle_connection_timeout (default 30s) that is used as a timeout in all three mentioned functions.

@sfc-gh-pfus sfc-gh-pfus requested a review from a team as a code owner February 6, 2026 09:43
@sfc-gh-pfus sfc-gh-pfus force-pushed the SNOW-3010074-fin-wait-2 branch from 00ef353 to e12f32a Compare February 6, 2026 09:54
@sfc-gh-pfus sfc-gh-pfus marked this pull request as draft February 6, 2026 10:01
@sfc-gh-pfus sfc-gh-pfus marked this pull request as ready for review February 6, 2026 10:10
Copy link

@sfc-gh-snoonan sfc-gh-snoonan left a comment

Choose a reason for hiding this comment

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

In theory this looks fine to me. Couple questions:

  1. Why keep the TTL argument at all? There's a constructor that doesn't require it.
  2. The intent of the TTL is to handle DNS re-resolution and to keep load balancing of different IPs on the backend well distributed. Today, these change infrequently enough that it shouldn't be a huge deal, and we don't publish a ton of IPs. However, I'm a bit worried this will eventually lead to hotspotting. Arguably, we get our load balancing across multiple clients rather than from the connection pool for one client though. So I will call it ok. I'd like to see a test around the DNS update behavior though.

@sfc-gh-pfus sfc-gh-pfus force-pushed the SNOW-3010074-fin-wait-2 branch 5 times, most recently from fd54a8b to 379695e Compare February 9, 2026 07:42
@sfc-gh-pfus sfc-gh-pfus force-pushed the SNOW-3010074-fin-wait-2 branch from 379695e to 692f2f5 Compare February 18, 2026 16:39
Copy link
Contributor

@sfc-gh-boler sfc-gh-boler left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments