Skip to content
This repository has been archived by the owner on Dec 6, 2023. It is now read-only.

Websocket shutdown #81

Merged
merged 4 commits into from Sep 17, 2021
Merged

Websocket shutdown #81

merged 4 commits into from Sep 17, 2021

Conversation

vividviolet
Copy link
Member

Fix #70

Tophat

Run make bootstrap; make build to set up

Client close connection

  1. Run ./shopify-extensions serve < testdata/shopifile.yml
  2. In a web browser, open a console and paste the following code
var ws = new WebSocket("ws://localhost:8000/extensions/");
ws.onClose = console.log;
ws.close(1000, "done");
  1. Verify that the console logs out a close event with the code 1000, the reason set to "done" and wasClean = true
    image

Server shut down

  1. Run ./shopify-extensions serve < testdata/shopifile.yml
  2. In a web browser, open a console and paste the following code
var ws = new WebSocket("ws://localhost:8000/extensions/");
ws.onClose = console.log;
  1. In the terminal running serve, kill the process. Note that there's a panic: send on closed channel thrown but this is not related to the websocket shutting down. We are currently not handling interrupts in the code to monitor so it tries to send a build error notification. This can be cleaned up in a later PR.
  2. Verify that the console logs out a close event with the code 1000, the reason set to "server shut down" and wasClean = true
    image

vividviolet and others added 4 commits September 16, 2021 01:02
…ket in order to shut down properly

Co-authored-by: t6d <konstantin@tennhard.net>
Co-authored-by: t6d <konstantin@tennhard.net>
…lled

- Ensure that the close code and message is sent correctly when close is intiated by the client
- Update the websocket tests to verify that notifications are not sent to closed connections
…tered otherwise we might get a channel close error if api.Notify is called during the shut down
@vividviolet vividviolet added this to In progress in Shopify CLI Extensions Sep 16, 2021
@vividviolet vividviolet marked this pull request as ready for review September 16, 2021 14:09
@@ -140,4 +177,11 @@ type extensionsResponse struct {
Version string `json:"version"`
}

type client struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but I think we should explore if client could wrap the connection and manage access to it. I'm thinking of

func newClient(conn *websocket.Conn, MessageHandler) *client {
  client := client{...}
  go client.receive()
  return &client
}
func (c *client) send(update StatusUpdate) {}
func (c *receive)
func (c *client) close()
type MessageHandler func(msg interface{})

Copy link
Member Author

Choose a reason for hiding this comment

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

I made a follow up issue #90

server := &http.Server{Addr: addr, Handler: api}

onInterrupt(func() {
api.Shutdown()
Copy link
Contributor

Choose a reason for hiding this comment

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

In future, we should pass a ctx with a deadline here as well. The shutdown should then be done as follows:

  1. Attempt graceful shutdown and wait until all connections have been closed
  2. Wait no longer than the deadline on the context permits

Copy link
Member Author

Choose a reason for hiding this comment

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

What would be set for the context deadline?

Shopify CLI Extensions automation moved this from In progress to Reviewed Sep 17, 2021
@vividviolet vividviolet merged commit a7afdef into main Sep 17, 2021
Shopify CLI Extensions automation moved this from Reviewed to Done Sep 17, 2021
@shopify-shipit shopify-shipit bot temporarily deployed to production November 2, 2021 21:57 Inactive
dmhenry pushed a commit that referenced this pull request Aug 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

On shutdown, all web socket connections should be terminated and the allocated port should be freed up. [Go]
2 participants