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

Add support for graphql-transport-ws with duplex ping-pong #1578

Merged
merged 2 commits into from
Nov 22, 2021

Conversation

zdraganov
Copy link
Contributor

@zdraganov zdraganov commented Aug 2, 2021

Extending this PR, included also the duplex ping-pong functionality.

How to use:

srv.AddTransport(transport.Websocket{
    PingPongInterval: 5 * time.Second,
    # Other options ...
})

Example Apollo client usage:

let activeSocket: unknown = null;
let timedOut: unknown = null;

const wsLink = new WebSocketLink({
  url: REACT_APP_GRAPHQL_WS_ENDPOINT as string,
  keepAlive: 10_000, // ping server every 10 seconds
  on: {
    ping: (received) => {
      if (!received) {
        timedOut = setTimeout(() => {
          if (activeSocket && activeSocket.readyState === WebSocket.OPEN) {
            activeSocket.close(4408, 'Request Timeout');
          }
        }, 5_000); // wait 5 seconds for the pong and then close the connection
      }
    },
    pong: (received) => {
      if (received) {
        clearTimeout(timedOut);
      }
    },
  },
});

@jaredly
Copy link
Contributor

jaredly commented Oct 13, 2021

@zdraganov did you make any substantive changes to #1507, or did you just extend it? It might be nice to merge the other first so I can review the extension in isolation.

@zdraganov
Copy link
Contributor Author

@jaredly I've just extended it. This one uses the feature of graphql-transport-ws protocol to have two-way keep-alive.
Feel free to merge the #1507 and then I will create a new one pointing the master branch.

@StevenACoffman
Copy link
Collaborator

@zdraganov I rebased and squash merged #1507. If you could rebase yours (and add that comment mentioned in #1507 ) I would appreciate it very much. Thanks!

@zdraganov zdraganov force-pushed the feature/IN-1281 branch 2 times, most recently from ba62201 to cc16324 Compare November 18, 2021 15:36
@zdraganov
Copy link
Contributor Author

Rebased with the master branch.

@coveralls
Copy link

coveralls commented Nov 19, 2021

Coverage Status

Coverage increased (+0.09%) to 70.408% when pulling 09f97c8 on Seven-of-Di:feature/IN-1281 into ae92c83 on 99designs:master.

Copy link
Contributor

@jaredly jaredly left a comment

Choose a reason for hiding this comment

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

code looks good! Could you add a test or two to websocket_test.go?

@StevenACoffman
Copy link
Collaborator

So when I run this:

go generate ./... && cd example/ && go generate ./...
git status

I get this:

deleted:    ../plugin/resolvergen/testdata/singlefile/out/resolver.go

And as a result, I then when I run this:

go test -race ./... && cd example && go test -race ./... && cd ..

I get a lot of failures which I assume are related. Sorry if I missed that when Jared and I reviewed it earlier (although maybe it's a new problem). Can you please take a look a this so we can get this merged? Thanks!

@zdraganov
Copy link
Contributor Author

Fixed the test, but the generation problem doesn't happens on my side - BigSlur M1
Can you check again?

@StevenACoffman StevenACoffman merged commit 213ecd9 into 99designs:master Nov 22, 2021
@StevenACoffman
Copy link
Collaborator

Huh, well it didn't happen when I refreshed everything and tried it again, so maybe I was on some other branch and they merged weird.

Thanks!

@zdraganov zdraganov mentioned this pull request Nov 22, 2021
2 tasks
@jaredly
Copy link
Contributor

jaredly commented Nov 22, 2021

(It would also be nice to have a test that is "client/server shuts down if pong isn't received within time limit")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants