Skip to content

Fix the issue where the 'Shutdown' method fails to properly exit. #255

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
ezynda3 merged 3 commits into from
May 10, 2025
Merged

Fix the issue where the 'Shutdown' method fails to properly exit. #255

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5daa583
Fix the issue where the 'Shutdown' method fails to properly exit.
May 8, 2025
d32050f
Fix the issue where the 'Shutdown' method fails to properly exit.
May 9, 2025
dd36a00
Merge branch 'mark3labs:main' into main
uppercaveman May 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions server/sse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,20 +281,15 @@ func (s *SSEServer) Start(addr string) error {
// Shutdown gracefully stops the SSE server, closing all active sessions
// and shutting down the HTTP server.
func (s *SSEServer) Shutdown(ctx context.Context) error {
s.mu.RLock()
srv := s.srv
s.mu.RUnlock()

if srv != nil {
if s.srv != nil {
Copy link

@JAORMX JAORMX May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain in the commit message why the read lock was causing the shutdown to not work properly? It is not entirely clear to me.

Copy link
Collaborator

@rwjblue-glean rwjblue-glean May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya, same. Unless the issue was that we couldn't get a read lock? But I'm not 100% sure why that would be the case...

Copy link
Collaborator

@rwjblue-glean rwjblue-glean May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think #263 explains why it deadlocks fairly well. Basically, upon Start being called we do the following:

s.mu.Lock() 
defer s.mu.Unlock()

So here in Shutdown() the s.mu.RUnlock() deadlocks (because the defer won't run until Start exits).

s.sessions.Range(func(key, value interface{}) bool {
if session, ok := value.(*sseSession); ok {
close(session.done)
}
s.sessions.Delete(key)
return true
})

return srv.Shutdown(ctx)
return s.srv.Shutdown(ctx)
}
return nil
}
Expand Down