fix: make Close() synchronously close xray instances#86
Merged
Conversation
added 2 commits
April 22, 2026 11:38
Add CloseImmediately() function that synchronously closes xray instances instead of relying on the delayed sweeper mechanism. This prevents goroutine and memory leaks when testing high volumes of vless/vmess/trojan proxies. The sweeper's 30-second DrainTimeout caused accumulation of xray instances when tests completed faster than the timeout, leading to resource exhaustion.
Change Close() to immediately close xray instances instead of marking them as draining with a 30-second delayed sweep. This prevents goroutine and memory leaks when testing high volumes of vless/vmess/trojan proxies where the previous delayed-close behavior caused resource accumulation.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR changes the xray Close() behavior from deferred, drain-based shutdown to an immediate, synchronous close and removal of instances from the servers map to prevent goroutine and memory leaks under high load. Sequence diagram for synchronous xray Close behaviorsequenceDiagram
participant Caller
participant XrayPackage as XrayPackage
participant ServersMap as ServersMap
participant Server
participant Instance
Caller->>XrayPackage: Close(proxyURL)
XrayPackage->>ServersMap: lookup proxyURL
alt server_found
ServersMap-->>XrayPackage: srv
XrayPackage->>Instance: Close()
Instance-->>XrayPackage: (returns)
XrayPackage->>ServersMap: delete proxyURL
ServersMap-->>XrayPackage: (removed)
else server_not_found
ServersMap-->>XrayPackage: not_found
XrayPackage-->>Caller: return
end
XrayPackage-->>Caller: return
Class diagram for updated xray Close and ServerclassDiagram
class XrayPackage {
+map[string]*Server servers
+Mutex mu
+Close(proxyURL string)
+tryCloseAndDelete(url string, srv *Server)
}
class Server {
+Instance *XrayInstance
+DrainedAt time_Time
}
class XrayInstance {
+Close() error
}
XrayPackage --> "*" Server : manages
Server --> XrayInstance : has
%% Highlight of Close behavior change
class CloseOld {
+Close(proxyURL string)
- set DrainedAt
- startSweeper()
}
class CloseNew {
+Close(proxyURL string)
- srv.Instance.Close()
- delete from servers
}
XrayPackage ..> CloseOld : replaced_by
XrayPackage ..> CloseNew : current_impl
State diagram for xray instance lifecycle before and after Close changestateDiagram-v2
state OldBehavior {
[*] --> Active
Active --> Draining: Close(proxyURL)
Draining --> Closed: sweeper_after_DrainTimeout
Closed --> [*]
}
state NewBehavior {
[*] --> Active
Active --> Closed: Close(proxyURL)
Closed --> [*]
}
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider cleaning up now-unused draining/sweeper logic (e.g.,
startSweeper,tryCloseAndDelete,DrainTimeoutconfig, and related comments likeCloseAll) so the lifecycle model is consistent and easier to reason about. - With
Closenow synchronously closing and deleting the server, double-check that any callers that still hold references tosrv.Instanceare properly synchronized to avoid use-after-close races or unexpected panics. - If there are scenarios where in-flight operations need a grace period, it may be worth making the synchronous vs. delayed close behavior explicit via separate methods or a parameter rather than hard-switching the existing
Closesemantics.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider cleaning up now-unused draining/sweeper logic (e.g., `startSweeper`, `tryCloseAndDelete`, `DrainTimeout` config, and related comments like `CloseAll`) so the lifecycle model is consistent and easier to reason about.
- With `Close` now synchronously closing and deleting the server, double-check that any callers that still hold references to `srv.Instance` are properly synchronized to avoid use-after-close races or unexpected panics.
- If there are scenarios where in-flight operations need a grace period, it may be worth making the synchronous vs. delayed close behavior explicit via separate methods or a parameter rather than hard-switching the existing `Close` semantics.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Update TestCloseRevivesServer to TestCloseRemovesServer since Close() now immediately removes the server from the map instead of marking it as draining. Update TestCloseIdempotent to check that server is removed from map rather than checking DrainedAt.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
- Coverage 12.71% 11.85% -0.87%
==========================================
Files 27 27
Lines 1565 2194 +629
==========================================
+ Hits 199 260 +61
- Misses 1351 1918 +567
- Partials 15 16 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
Change
Close()to immediately close xray instances instead of marking them as draining with a 30-second delayed sweep.Problem
The previous behavior marked xray instances as "draining" but only closed them after
DrainTimeout(30 seconds). When testing high volumes of vless/vmess/trojan proxies, this caused:Solution
Make
Close()synchronously close the xray instance and remove it from the servers map immediately.Test plan
Summary by Sourcery
Bug Fixes: