- Notifications
You must be signed in to change notification settings - Fork1k
fix: ensure wsproxyMultiAgent is closed when websocket dies#11414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix: ensure wsproxyMultiAgent is closed when websocket dies#11414
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coadler commentedJan 4, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
In this stack: This stack of pull requests is managed by Graphite.Learn more about stacking. |
50c92b4 to12f1878CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good work tracking down the bug!
I don't see any new tests though that will catch and prevent a regression, so please add
| gofunc() { | ||
| defercancel() | ||
| dec:=json.NewDecoder(nc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's worth dropping an INFO log here.
See this comment inline onGraphite.
| }).Init() | ||
| gofunc() { | ||
| <-ctx.Done() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I'm understanding this correctly, we're depending on the fact that the reader goroutine below cancels the context on a failed read.
I think we shouldalso tear down the multi-agent on a failed write of subscription messages. It's unlikely that we'd have a failure that leaves the connection half-open (e.g. for reads but not writes), but such things are possible and you don't want the proxy limping on unable to subscribe to new agents.
| ticker:=time.NewTicker(15*time.Second) | ||
| deferticker.Stop() | ||
| for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The `SingleTailnet` behavior only checked to see if the `MultiAgent` wasclosed, but the websocket error was not being propogated into the`MultiAgent`, causing it to never be swapped for a new working one.
12f1878 to72ad811Compare
Uh oh!
There was an error while loading.Please reload this page.
The
SingleTailnetbehavior only checked to see if theMultiAgentwasclosed, but the websocket error was not being propogated into the
MultiAgent, causing it to never be swapped for a new working one.Fixes#11401
Before:
After: