Add TCP connection pooling for NUT server client#405
Open
Brandawg93 wants to merge 7 commits intomainfrom
Open
Add TCP connection pooling for NUT server client#405Brandawg93 wants to merge 7 commits intomainfrom
Brandawg93 wants to merge 7 commits intomainfrom
Conversation
Reuses idle TCP connections across sequential NUT protocol calls instead of opening and closing a fresh socket per command. This reduces handshake overhead on operations like getDevice() that fan out to many methods (LIST VAR, GET DESC × N, GET TYPE × N, LIST CMD, GET UPSDESC) and keeps the number of concurrent sockets bounded to prevent EventEmitter listener buildup. - Add NutConnectionPool (src/server/nut-pool.ts): module-level singleton keyed by host:port, LIFO idle pool capped at 3 per server, idle sweep timer with 30s timeout, drain() for clean shutdown - Replace getConnection() in Nut with getAcquiredConnection() that tries the pool first for unauthenticated sessions; authenticated sessions (checkCredentials=true) always use a fresh socket and are destroyed after use (LOGOUT + close) to avoid sharing LOGIN state - Add releaseConnection(): returns successful unauthenticated connections to the pool; destroys errored or authenticated ones - Update getData() to use getAcquiredConnection/releaseConnection so the shared socket pattern (passing socket to getCachedVarDescription etc.) is preserved while the connection is pooled at the outer scope - Add .claude to .gitignore Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds TCP connection pooling to the NUT (Network UPS Tools) client to reduce connection churn and improve performance for frequent variable queries, while keeping authenticated sessions isolated from pooling.
Changes:
- Introduces
NutConnectionPoolto reuse idle TCP sockets perhost:portwith max size + idle timeout sweeping. - Refactors
Nutconnection lifecycle to acquire/release pooled connections for unauthenticated commands and force-close authenticated sessions. - Adds unit tests covering pool behavior and validates
Nutintegration with pooling.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/server/nut.ts |
Refactors command execution and getData() to acquire/release pooled connections; authenticated connections bypass pooling. |
src/server/nut-pool.ts |
Implements host/port-based connection pool with LIFO reuse, idle sweeping, and drain support. |
__tests__/unit/server/nut.test.ts |
Adds tests ensuring pooled reuse, correct release/close behavior on success/error, and authenticated bypass. |
__tests__/unit/server/nut-pool.test.ts |
Adds a comprehensive test suite for pool acquire/release/drain, LIFO behavior, and idle sweep behavior. |
.gitignore |
Ignores .claude directory/file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Extract makePoolSocket factory in nut.test.ts to eliminate repeated mock socket object literals across Connection Pool test cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fix socket leak: wrap checkCredentials() in try/catch in the
authenticated branch of getAcquiredConnection so the socket is always
closed if credential validation throws
- Track hasError in getData() and pass to releaseConnection so broken
sockets are not returned to the pool after a mid-request failure
- Update releaseConnection docstring to match actual behaviour (overflow
connections are also returned to the pool, not closed directly)
- Remove empty buckets from the pools map in acquire(), sweepIdle(), and
drain() to prevent unbounded map growth
- Fix drain(host, port) to only clear the global sweep timer when all
buckets have been drained; scoped drains leave the timer running if
other buckets still have idle connections
- Switch process.on('exit') to 'beforeExit' so the async drain() call
can actually complete before the process exits
- Deduplicate mock socket construction in Connection Pool tests behind
a makePoolSocket factory that accepts a custom readAll mock
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…threshold Extract openSocket() in nut.ts to eliminate the repeated connect/error-wrap pattern across getAcquiredConnection and checkCredentials. Extract mockGetDevicesWithUps() helper in nut.test.ts to deduplicate the repeated getDevices spy setup across checkCredentials and Connection Pool tests. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/server/nut.ts:185
getCommand()converts anyreadAll()failure forLIST VAR ...intoDEVICE_UNREACHABLE, which means transport errors (timeouts/connection resets) can be treated as a non-error and the underlying socket may be returned to the pool. If the socket still reportsisConnected() === true, this can poison the pool with a connection that will keep timing out/failing. Consider distinguishing NUT protocolERR ...responses from transport errors (or move theDEVICE_UNREACHABLEfallback intogetData()so you can still mark the connection as errored and close it) so sockets that hit real I/O failures are not pooled.
const data = await conn.socket.readAll(command, until).catch((error) => {
this.debug.warn('Command failed, handling fallback', { command, error: error.message })
if (command.startsWith('LIST VAR')) {
return upsStatus.DEVICE_UNREACHABLE
}
throw error
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use process.once('beforeExit') in nut-pool.ts to avoid duplicate listener
registration on hot reload. In getData(), mark hasError=true when getCommand
returns DEVICE_UNREACHABLE so transport-level failures destroy the socket
instead of returning a potentially broken connection to the pool.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
New Connection Pool (
nut-pool.ts)NutConnectionPoolclass to manage reusable TCP connections per host:port pairdrain()method for graceful shutdownUpdated NUT Client (
nut.ts)getAcquiredConnection()method that tries pool first, then creates fresh socket if neededreleaseConnection()method that returns healthy connections to pool or closes on errorgetData()now reuses pooled connections across multiple variable lookupsTests
NutConnectionPool(211 lines): acquire/release/drain, LIFO behavior, stale socket handling, idle sweep, pool capacity limitsNuttests (112 new lines): verify pool reuse, release on success/error, authenticated bypass, socket passing to helpersConfiguration
.claudeto.gitignoreBenefits