[471] - Close underlying HTTP Client on closing Connection

[Varun0157]

(Addresses #471).

What type of PR is this?

• Bug Fix

Description

Here, we resolve a few resource leaks with regards to TCP connections remaining open even after the Connection is closed:

• Currently, on session closure, the thrift backend invokes self._transport.close() which simply releases a connection back to the underlying pool. It does not close the pool itself. This leads to the TCP connection not being closed properly and a dangling CLOSE_WAIT state as pointed out in the above issue.
  Thus, we rename _transport.close() to the more appropriate _transport.release_connection() and implement a new _transport.close() that actually closes the underlying pool.
  We invoke this method on SEA backend session closure as well.
• Currently, the close() method of the TelemetryClient does not close the unified HTTP client instantiated by it. We add code to close this as well.

How is this tested?

• Unit tests
• E2E Tests
• Manually
  • using the same scripts mentioned in the description of TCP connection not closing properly leading to dangling CLOSE_WAIT state and system overload #471 - the connection is now closed properly and there is no dangling CLOSE_WAIT.
  • testing with an invalid token - before the addition of the http_client.close() in the TelemetryClient, a similar resource leak as pointed out above was observed, that has now been mitigated.
• N/A

Related Tickets & Documents

#471

[Varun0157]

@jprakash-db / @jayantsing-db / @vikrantpuppala
This PR addresses #471 and fixes a few other related issues. Can't seem to add reviewers, so pinging you here.
Thanks!

[vikrantpuppala]

Thanks for making these fixes! could you update from the latest main (just made significant changes to use a unified http client)

[jayantsing-db]

why is a session and http client closed together? I feel it is easier to imagine the backend module as a function module for interacting with SQL EXEC. So, with that perspective, there should be minimal state in this and the backend should receive resources by higher level classes and those higher level classes should take care of closing the resources (for example, the connection/cursor should pass a http client to this and close the http client. Same goes for session: cursor maintains the session obtained functionally through this backend and should take care of closing the session). Please let me know if this doesn't look okay.

[Varun0157]

I agree with the philosophy overall, but I think passing the HTTP client into the backend from a higher-level object is likely infeasible right now since both backends have distinct, custom HTTP clients that expose different methods and are processed differently. Until we unify them, it makes sense to make the backend responsible for it's own HTTP Client instantiation, as a result of which it should be responsible for closing it as well.

Both HTTP Clients do have largely the same functionality, so unifying them (for the most part) could be a separate PR, at which point we can pass in the HTTP client from the Session class.
Let me know if I should do it in this PR instead.

Note that the pattern of instantiating the required HTTP client in the constructor of the concrete backend is common across Thrift and SEA.

[Varun0157]

Thoughts?
@jayantsing-db / @vikrantpuppala / @jprakash-db

[Copilot]

Pull request overview

This PR addresses resource leaks where TCP connections remain open after closing database connections (#471). The changes ensure proper cleanup of HTTP connection pools and pending operations.

Key Changes:

• Renamed THttpClient.close() to release_connection() to clarify its purpose
• Implemented new close() methods that properly close connection pools
• Added tracking of pending telemetry futures to ensure graceful shutdown

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/databricks/sql/auth/thrift_http_client.py	Renamed close to release_connection, added proper close implementation
src/databricks/sql/backend/thrift_backend.py	Updated to use release_connection instead of close
src/databricks/sql/backend/sea/utils/http_client.py	Changed pool.clear() to pool.close()
src/databricks/sql/backend/sea/backend.py	Added HTTP client closure on session close
src/databricks/sql/telemetry/telemetry_client.py	Added pending futures tracking and proper HTTP client closure
tests/unit/test_thrift_backend.py	Added assertion to verify transport close is called
tests/unit/test_sea_backend.py	Added assertion to verify HTTP client close is called

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Consider checking if the pool supports the close() method before calling it. urllib3 PoolManager has a close() method, but if this code supports multiple pool implementations, calling clear() might be safer as a fallback. If all supported pools guarantee close() exists, this is fine, but the change from clear() to close() should be verified against all pool types used.

Suggested change

	if self._pool:
	if self._pool and hasattr(self._pool, "close"):