add websocket support to transit relay client #49
Conversation
websocket: switch to nhooyr.io/websocket
GOOS=js GOARCH=wasm go build -o wormhole.wasm -tags js,wasm wasm/module/main.go produces a wasm file.
wasm build tags
bryanchriswhite
force-pushed
the
upstream-websocket-relay
branch
4 times, most recently
from
9a97030 to
2427d6c
Compare
|
Are you planning on opening a PR in the magic wormhole protocols repo to get this transport officially supported? https://github.com/magic-wormhole/magic-wormhole-protocols |
|
Yes we are, we have been in touch with the original author and our team now has access to the official magic-wormhole org. |
bryanchriswhite
force-pushed
the
upstream-websocket-relay
branch
3 times, most recently
from
4c1f39a to
a47e554
Compare
bryanchriswhite
force-pushed
the
upstream-websocket-relay
branch
2 times, most recently
from
d9d4f73 to
edad403
Compare
There was a problem hiding this comment.
I gave the code a brief look and left you a few comments on the immediate things I noticed (this is not a comprehensive review).
Overall this change looks pretty good to me. I would like to see some consensus on the protocol addition with the broader Magic Wormhole community in: https://github.com/magic-wormhole/magic-wormhole-protocols
| failChan <- addr | ||
| return | ||
| } | ||
| case "ws", "wss", "http", "https": |
There was a problem hiding this comment.
What's the reason to allow http{s,} for a websocket transport?
There was a problem hiding this comment.
I'm not absolutely sure this is necessary. My thinking was that the other end (relay in this case) determines what protocol scheme it wants to use so it would be polite to be flexible on this end to make integration with relay implementations easier.
| // DefaultTransitRelayAddress is the default transit server to ues. | ||
| DefaultTransitRelayAddress = "transit.magic-wormhole.io:4001" | ||
| // DefaultTransitRelayURL is the default transit server to ues. | ||
| DefaultTransitRelayURL = "tcp:transit.magic-wormhole.io:4001" |
There was a problem hiding this comment.
These are breaking changes, which would require us to bump the major version number of the module. I'd really prefer to make these changes in a backwards compatible way.
I would suggest we add the new fields TransitRelayURL and DefaultTransitRelayURL but leave the existing fields and mark them as deprecated. Then in the code, if the old fields are set their values would be used, otherwise use the new fields.
This strategy would be similar to how net/http has handled deprecated fields.
|
@psanford thanks for the feedback, and my apologies for the delay on my end. I'll be getting back to this beginning next week. |
bryanchriswhite
force-pushed
the
upstream-websocket-relay
branch
from
4c33e3f to
ec67543
Compare
|
@psanford We added deprecation warnings to the old fields as suggested. Regarding the wormhole-protocols repo, File Tansfers -> Future Extensions mentions WebSockets. Is there some specific change you'd like to see in that repo? |
We've opened this PR to document the transit relay protocol changes. |
This significantly improves the performance. My testing indicates that
archiving is up to about about 2.5x faster and unarchiving around 1.3x
faster.
Closes: #28 [via git-merge-pr]
Closes: #29 [via git-merge-pr]
Closes: #31 [via git-merge-pr]
This allows for shell completions of recv codes. A new command `shell-completion` has been added with will generate the necessary shell config to enable completions for wormhole-william. See `wormhole-william shell-completion --help` for more details. Upgrade `spf13/cobra` to a new enough version to get the recent shell completion enhancements.
We see occasional failures on Github CI for this test where we haven't gotten any agent strings back. This appears to be a race condition between the server ack'ing the message and then saving the user agent string. Move the ack after the save to ensure its there in the test. Closes: #36 [via git-merge-pr]
There's was race between waitFor() and collect() where collect() could close the msgCollector before waitFor got past that check. In that case waitFor() wouldn't get the actual error it should return to the caller. We now send the error on the done channel so later call to waitFor can still receive it. Example failure: https://github.com/psanford/wormhole-william/runs/2103326907? Closes: #39 [via git-merge-pr]
Canceling the context passed to Receive() will close the connection and error on the next .Read(). Closes: #38 [via git-merge-pr]
Canceling the context will now cancel an in-flight file transfer. This should cover most cases. There might still be some edge cases where cancellation doesn't work. Closes: #40 [via git-merge-pr]
- Include info about shell completions. - Add link to wormhole-william-mobile Closes: #41 [via git-merge-pr]
These tests make sure files are all gofmt'ed and that a `go mod tidy` doesn't produce any changes to the `go.mod` or `go.sum` files. I want this information exposed in PRs. These tests are behind the `ci` build tag. This is mainly because the go mod tidy test is potentially destructive (it will update your go.mod/sum files) and also cares about the current state of your git working tree. There's also some issues with the gofmt test running on windows so we just run these tests in linux in the github action. Closes: #47 [via git-merge-pr]
Closes: #43 [via git-merge-pr]
This improves entropy compression for deflate to get slightly better
compression ratios with improved speeds. This also cleans up a lot of
unnecessary type conversions for slightly better performance.
Closes: #48 [via git-merge-pr]
This commit incorporates a couple code cleanups, with some being more important than others. Below is a summary of what has been done: - Replaced an unnecessary `io.TeeReader` and `countWriter` with the bytes written by `io.Copy` (should be marginally faster than before). - Fixed the same `prefixPath` being recalculated on each directory iteration (should also result in a small performance improvement). - Move `variable = variable + number` to `variable += number`. Closes: #35 [via git-merge-pr]
bryanchriswhite
force-pushed
the
upstream-websocket-relay
branch
from
793affa to
8881adc
Compare
|
Was this intended to be closed? This is something that would have been great to see merged. |
|
I'd like to take the opportunity to point to magic-wormhole/magic-wormhole-protocols#16 for encoding the new transit hints. Feedback welcome. |
@Jacalz yes, we're re-organizing our fork, including merging upstream changes. I'll re-open this after we've sorted out our fork, perhaps it will get some more attention then. |
|
Reorganized and re-opened as #63 |
Our team intends to use this as our project's client library in the browser via a WASM wrapper. This PR adds support for websocket transport for the transit relay. The transit relay needs to use a transport that is compatible with browser environments. Websockets seem to be the obvious solution here; foregoing any webrtc-based approaches (which we're not interested in pursuing right now).
The the transit relay hasn't canonically supported a browser compatible transport. We have implemented such a ws relay transport here and this PR updates the client accordingly.