Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: Let SSE client reconnect after EOF #28

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Let SSE client reconnect after EOF #28

wants to merge 1 commit into from

Conversation

mwain
Copy link

@mwain mwain commented Mar 13, 2024 •
edited
Loading

Features and Changes

Fixes leaking go routine - github.com/ian-ross/sse/v2.(*Client).readLoop

119 @ 0x446d76 0x40f00e 0x40ebbd 0x168247e 0x47b3c1
#	0x168247d	github.com/ian-ross/sse/v2.(*Client).readLoop+0x25d

Calling Unsubscribe in the OnDisconnect callback causes the errorChan in the patched sse package to never be consumed. Causing readLoop go routine to never end.

This allows the sse client to reconnect. A custom ReconnectStrategy is used to ensure it reconnects indefinitely.

  • Closes (add link to issue here)

Dependencies

Testing

Screenshots

@mwain mwain changed the title fix: Let SSE client to reconnect after EOF fix: Let SSE client reconnect after EOF Mar 13, 2024
@mwain
fix: Let SSE client reconnect after EOF
e946712 
Fixes leaking go routine - `github.com/ian-ross/sse/v2.(*Client).readLoop`
```
119 @ 0x446d76 0x40f00e 0x40ebbd 0x168247e 0x47b3c1
```

Calling `Unsubscribe` in the `OnDisconnect` callback causes the [errorChan](https://github.com/ian-ross/sse/blob/master/client.go#L166)
in the patched sse package to never be consumed. Causing [readLoop](https://github.com/ian-ross/sse/blob/master/client.go#L220) go routine to never end.

This allows the sse client to reconnect. A custom `ReconnectStrategy` is
used to ensure it reconnects indefinitely.
@mwain mwain force-pushed the leaky-goroutines branch from 8258ca2 to e946712 Compare March 13, 2024 20:43
@atombender
Copy link

@ian-ross @huantt Any movement on this? The goroutine leak is quite serious, and we had to fork this library to use this PR in order to fix the issue.

Screenshot 2024-06-18 at 11 06 35

@ian-ross
Copy link
Collaborator

@ian-ross @huantt Any movement on this? The goroutine leak is quite serious, and we had to fork this library to use this PR in order to fix the issue.

Sorry I can't help there, @atombender, I'm no longer working for GrowthBook.

@adomaskizogian
Copy link

@ian-ross @huantt Any movement on this? The goroutine leak is quite serious, and we had to fork this library to use this PR in order to fix the issue.

Sorry I can't help there, @atombender, I'm no longer working for GrowthBook.

Do you know if anyone is? Is there someone to ping? We're seriously considering adopting growthbook but looking how little attention it receives from maintainers is concerning. Especially that there are multiple pull request proposals to address the issues and they are just hanging stale.

@ian-ross
Copy link
Collaborator

@adomaskizogian Most of the support is done on Slack. I would try there. Both Graham McNicoll (the CEO) and Jeremy Dorn (the CTO) are active there.

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mwain @atombender @ian-ross @adomaskizogian