Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(wsapi): Gracefully close the websocket connection #1347

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

Conversation

armsnyder
Copy link
Contributor

This PR removes a time.Sleep(1 * time.Second) from Session.Close().

Instead, I implemented the websocket close handshake described in this comment: gorilla/websocket#448 (comment)

The reason I would like to see this change is that the time.Sleep was causing my unit tests to run slowly.

I tested this change by running this program a few times:

package main

import (
	"github.com/bwmarrin/discordgo"
	"github.com/samber/lo"
)

func main() {
	session := lo.Must(discordgo.New(Getenv("BOT_TOKEN")))
	session.LogLevel = discordgo.LogDebug
	lo.Must0(session.Open())
	lo.Must0(session.Close())
}

wsapi.go Show resolved Hide resolved
@FedorLap2006 FedorLap2006 added gateway Gateway related issues and pull requests improvement Improvement of existing code and features labels Apr 2, 2023
@FedorLap2006 FedorLap2006 added this to the v0.28.0 milestone Apr 2, 2023
@@ -951,3 +938,36 @@ func (s *Session) CloseWithCode(closeCode int) (err error) {

return
}

func (s *Session) doCloseHandshake(closeCode int) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

After a bit of thinking. It might be a good idea to also move this function away from Session into util.go, with websocket prefixed name (e.g. doWebsocketCloseHandshake). Since we can reuse it in our voice websocket handling code.

}

// Wait for Discord to actually close the connection.
// To prevent ping, pong and close handlers from setting the read deadline,
Copy link
Collaborator

Choose a reason for hiding this comment

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

setting read deadline

I'm not quite sure what do you refer to. The handlers in websocket by default don't extend the read deadline. And we don't override them (well, close is overriden, but is noop)

@FedorLap2006 FedorLap2006 added the high priority Issue or PR with high priority of merge label Apr 11, 2023
@FedorLap2006 FedorLap2006 removed the high priority Issue or PR with high priority of merge label Dec 24, 2023
@FedorLap2006 FedorLap2006 removed this from the v0.28.0 milestone Dec 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gateway Gateway related issues and pull requests improvement Improvement of existing code and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants