clean up pools after 24hours inactivity#6545
Conversation
Mzack9999
added
the
Type: Bug
WalkthroughReplaced a plain HTTP client map with an eviction-aware sync-lock map (24‑hour TTL) for Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Engine as Engine / Runner
participant Pool as mapsutil.SyncLockMap\n(24h TTL, 12h interval)
participant Client as retryablehttp.Client
rect rgba(0,128,96,0.06)
Note over Pool: Eviction operates asynchronously\nidle entries removed after 24h
end
Engine->>Pool: Get client by config key
alt client exists
Pool-->>Engine: return existing Client
else client missing
Pool->>Client: create new Client
Pool-->>Engine: return new Client
end
Engine->>Client: perform HTTP requests
Note right of Pool: Evicted entries are cleaned up\nand can be recreated on demand
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
pkg/protocols/**/*.go📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)pkg/protocols/common/protocolstate/state.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(2 hunks)pkg/protocols/common/protocolstate/state.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/protocols/common/protocolstate/state.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Each protocol implementation must provide a Request interface with methods Compile(), ExecuteWithResults(), Match(), and Extract()
Files:
pkg/protocols/common/protocolstate/state.go
🧬 Code graph analysis (1)
pkg/protocols/common/protocolstate/state.go (1)
pkg/protocols/common/protocolstate/dialers.go (1)
Dialers(13-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (ubuntu-latest)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
🔇 Additional comments (3)
pkg/protocols/common/protocolstate/state.go (1)
8-8: LGTM!The time import is necessary for the 24-hour eviction duration constant.
go.mod (2)
112-112: No issues found with the pseudo-version dependency.The commit
27dede511f26exists in the projectdiscovery/utils repository and corresponds to PR #686 ("Adding Evict Functionality"). No security advisories were detected. The dependency update is valid.
306-306: No action required—update fixes a moderate security vulnerability.The version v2.2.1 exists and the update from v2.1.0 is beneficial. A moderate severity DoS vulnerability ("unrestricted RAR dictionary sizes") affected versions < 2.2.0 and was fixed in v2.2.0. The PR updates to v2.2.1, which includes this security patch.
| httpClientPool := mapsutil.NewSyncLockMap( | ||
| // evicts inactive httpclientpool entries after 24 hours | ||
| // of inactivity (long running instances) | ||
| mapsutil.WithEviction[string, *retryablehttp.Client](24 * time.Hour), | ||
| ) | ||
|
|
||
| dialersInstance := &Dialers{ | ||
| Fastdialer: dialer, | ||
| NetworkPolicy: networkPolicy, | ||
| HTTPClientPool: mapsutil.NewSyncLockMap[string, *retryablehttp.Client](), | ||
| HTTPClientPool: httpClientPool, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that evicted HTTP clients are properly cleaned up.
The implementation correctly creates a SyncLockMap with 24-hour eviction for HTTP client pooling. However, retryablehttp.Client instances may hold underlying resources (connections, goroutines) that require explicit cleanup.
Verify whether the eviction automatically closes the client's resources or if an eviction callback is needed:
If retryablehttp.Client requires explicit cleanup, consider adding an eviction callback:
httpClientPool := mapsutil.NewSyncLockMap(
mapsutil.WithEviction[string, *retryablehttp.Client](24 * time.Hour),
mapsutil.WithOnEviction[string, *retryablehttp.Client](func(key string, client *retryablehttp.Client) {
if client != nil {
client.HTTPClient.CloseIdleConnections()
}
}),
)🏁 Script executed:
#!/bin/bash
# Search for Close methods and resource management in retryablehttp.Client
echo "Checking retryablehttp.Client for cleanup methods..."
ast-grep --pattern $'type Client struct {
$$$
}'
echo -e "\nSearching for Close or cleanup methods..."
rg -nP --type=go -C3 'func.*Close|func.*Shutdown' -g 'retryablehttp*'
echo -e "\nChecking mapsutil.SyncLockMap eviction callback support..."
ast-grep --pattern $'func NewSyncLockMap($$$) {
$$$
}'
rg -nP --type=go -C5 'eviction.*callback|OnEvict' -g '**/mapsutil/**' -g '**/utils/maps/**'Length of output: 2638
2 participants
Proposed changes
Closes #6329
Checklist
Summary by CodeRabbit