websocket: change lib to nhooyr.io/websocket

[Hellysonrp]

Changes

• Removed gorilla/websocket
• Added nhooyr.io/websocket

I had to update some gRPC and protobuf packages to run the tests in my environment.

This PR fixes #713, #543, and #664. Might fix other issues.

Verification

Tested using chrome.
Here is a screenshot of one of my tests (after the change and before the change, respectively):

In this test, the testserver keeps logging the active goroutines number every 5 seconds (didn't commit it, as it needs someone to look at the log).
It shows that the goroutine leak is fixed.

I'm opening this PR to see if the other tests will pass.
I didn't add tests for the mentioned issues because I honestly don't know how.

[improbable-prow-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign
You can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[johanbrandhorst]

Some nits, the bigger question is whether we're OK with this potentially dangerous change. I had a look at nhooyr.io/websocket and it looks legit, and this change is surprisingly small. It'll be interesting to see if all the tests still pass, but I'm tentatively supportive of this change, especially in light of gorilla/websocket#370.

CC @MarcusLongmuir

[pcj]

Anecdotally appears to be working in my (bazel build based) environment. These are the go_repository rules I used:

    go_repository(
        name = "com_github_improbable_eng_grpc_web",
        importpath = "github.com/improbable-eng/grpc-web",
        urls = ["https://github.com/Hellysonrp/grpc-web/archive/9a1da9358bf863017c5b9b9db6f83c9c9f7fd789.tar.gz"],
        strip_prefix = "grpc-web-9a1da9358bf863017c5b9b9db6f83c9c9f7fd789/",
        build_file_generation = "on",
        build_file_proto_mode = "disable",
    )

    # Release: v1.8.6
    # TargetCommitish: c9f314abd11b749d43bb61fd214171f8bb4e4173
    # Date: 2020-05-10 12:46:20 +0000 UTC
    # URL: https://github.com/nhooyr/websocket/releases/tag/v1.8.6
    # Size: 51231 (51 kB)
    go_repository(
        name = "io_nhooyr_websocket",
        importpath = "nhooyr.io/websocket",
        urls = ["https://github.com/nhooyr/websocket/archive/c9f314abd11b749d43bb61fd214171f8bb4e4173.tar.gz"],
        strip_prefix = "websocket-c9f314abd11b749d43bb61fd214171f8bb4e4173/",
    )
`

[johanbrandhorst]

Tests seem genuinely unhappy, would be good if someone could take a look.

[pcj]

@Hellysonrp ping

[Hellysonrp]

@pcj Sorry for the delayed response.
I have no idea why the tests failed. I'm not used to work with tests.
The logs don't seem to help either. Most of the tests just hang after Karma starts.
I honestly don't know how to help.

[johanbrandhorst]

I think this significant of a change will not be able to be merged until someone with more understanding of the tests can investigate why they failed. Unfortunately, that's probably only @MarcusLongmuir or @easyCZ. I appreciate that this looks like it should work on the surface but the tests, while flake, are supposed to give us a good indication of browser compatibility, so when they all fail that is worrying.

[pcj]

I was curious so I checked out the PR and ran the tests again, confirming they do pass locally. I'm also not sure about the tests failing in CI.

I've been using this branch for a side project and have not seen any issues so I would continue to push to get this in.

@Hellysonrp do you want to make another change and push that to see if the test failure is reproducible? For example, maybe refactor that string grpc-websockets into a const?

[pcj]

@Hellysonrp perhaps also add GODEBUG=http2debug=1 to .travis.yml env/global section so we can get a sense of what the server is seeing in CI. There may be other env vars that will enable better debugging, @johanbrandhorst might know.

[Hellysonrp]

# github.com/improbable-eng/grpc-web/integration_test/go/_proto/improbable/grpcweb/test
go/_proto/improbable/grpcweb/test/test.pb.go:14:2: can't find import: "google.golang.org/grpc/codes"

I think that is the problem.
I'll try to fix it.

[johanbrandhorst]

Hm, implies that there's a dependency management issue. I briefly looked into what it would take to migrate this repo to modules a while back and it's not trivial, but I think it has to be done eventually. It may be the tests won't work until we do.

[pcj]

It may be that you upgraded protobuf/grpc dependencies in this PR but did not regenerate the (generated but checked into vcs) protos. It is mandatory to upgrade those dependencies?

[Hellysonrp]

Maybe... In my environment, it didn't work until I upgraded it, but this could be a conflict with my other projects.
Let me try to revert the package upgrade.

[Hellysonrp]

It seems like all the browser tests are still hanging after Karma starts.
The browser is not being launched.
At least the nodejs test passed...

[johanbrandhorst]

Sigh, we've had nothing but problems with these tests. I'm inclined to accept this, given that @pcj has tested it in anger and the tests pass locally. The websocket transport has always been advertised as experimental so if it does cause a bug down the line that's acceptable, given that this fixes a very real race condition.

I will leave this PR for another week to give @MarcusLongmuir and @easyCZ time to object, but if I hear nothing, I will merge it. Thanks for your patience and help @Hellysonrp and @pcj.

[johanbrandhorst]

Note that @MarcusLongmuir has reworked the tests using CircleCI and SauceLabs in #823. We'll likely be merging that first, which might fix this test issue. If you'd like, you could try rebasing on that now, but note that there may still be changes to it, so it would be fine to wait, too 😄.

[pcj]

Anyone know if the tests ran? Was expecting to see checks here after that last push.

[johanbrandhorst]

Lets see if I can coerce the tests...

[johanbrandhorst]

@MarcusLongmuir doesn't look like tests are being triggered from third party PRs, any ideas?

[MarcusLongmuir]

I've just configured CircleCI to build forked PRs as I missed that when I merged the change. Can you please make a no-op commit to trigger it please?

[johanbrandhorst]

Reopening the PR seems to have triggered it 😁.

[johanbrandhorst]

This is awesome, thanks to everyone involved!