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

Websocket Transport #593

Merged
merged 25 commits into from
Aug 3, 2021
Merged

Websocket Transport #593

merged 25 commits into from
Aug 3, 2021

Conversation

Menduist
Copy link
Contributor

@Menduist Menduist commented Jun 24, 2021

closes #407

Full WIP

Edit: green ci!

@Menduist Menduist changed the base branch from unstable to transportsimprove June 25, 2021 13:40
@Menduist Menduist force-pushed the wsv1 branch 2 times, most recently from 472b352 to 6a71b20 Compare June 28, 2021 15:31
@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #593 (d5dc2bc) into unstable (c12f00c) will increase coverage by 0.02%.
The diff coverage is 90.90%.

❗ Current head d5dc2bc differs from pull request most recent head fcd98f1. Consider uploading reports for the commit fcd98f1 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##           unstable     #593      +/-   ##
============================================
+ Coverage     81.96%   81.99%   +0.02%     
============================================
  Files            59       60       +1     
  Lines         11579    11725     +146     
============================================
+ Hits           9491     9614     +123     
- Misses         2088     2111      +23     
Impacted Files Coverage Δ
libp2p/wire.nim 48.71% <ø> (ø)
libp2p/transports/wstransport.nim 90.55% <90.55%> (ø)
libp2p/multiaddress.nim 76.91% <100.00%> (+0.07%) ⬆️
libp2p/transports/tcptransport.nim 79.56% <100.00%> (-1.33%) ⬇️
libp2p/protocols/pubsub/pubsub.nim 81.93% <0.00%> (-1.30%) ⬇️
libp2p/protocols/pubsub/gossipsub.nim 83.51% <0.00%> (-0.80%) ⬇️
libp2p/crypto/rsa.nim 78.22% <0.00%> (-0.39%) ⬇️
libp2p/dial.nim 93.90% <0.00%> (+0.53%) ⬆️

Base automatically changed from transportsimprove to unstable June 30, 2021 08:59

try:
let
transp = await self.httpserver.accept()
Copy link
Contributor

Choose a reason for hiding this comment

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

Lots of potential to unify with tcp transport accept, specially after we've enabled multiple address listening.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, when #598 will be more stable I'll rebase this PR on it and add the required changes :)

@Menduist Menduist mentioned this pull request Jul 14, 2021
dryajov and others added 3 commits July 15, 2021 11:01
* change log level

* fixed issue related to stopping

some cosmetic cleanup

* use `allFutures` to stop/close things

Prevent potential race conditions when stopping two or more transports

* misc

* point websock to server-case-object branch
@Menduist Menduist marked this pull request as ready for review July 16, 2021 14:52
@Menduist
Copy link
Contributor Author

Well, the interop test with go was surprisingly uneventful, so this PR should be pretty good?
I'm calling it ready for review, and we'll see if we merge it before #598, since #598 is currently stuck and no one actually need it ATM

@dryajov
Copy link
Contributor

dryajov commented Jul 18, 2021

I didn't see interop tests for websockets in the testinterop.nim file, we should definitelly have those added.

Never mind, they are there 👍

tests/testinterop.nim Outdated Show resolved Hide resolved
@dryajov
Copy link
Contributor

dryajov commented Jul 18, 2021

LGTM, besides the missing websocket -> daemon test, lets add it please.

dryajov
dryajov previously approved these changes Jul 19, 2021
@Menduist Menduist merged commit af3be79 into unstable Aug 3, 2021
@Menduist Menduist deleted the wsv1 branch August 3, 2021 13:48
@Menduist Menduist added this to Q3 2021 in Roadmap Aug 31, 2021
dryajov added a commit that referenced this pull request Sep 8, 2021
* start of websocket transport

* more ws tests

* switch to common test

* add close to wsstream

* update ws & chronicles version

* cleanup

* removed multicodec

* clean ws outgoing connections

* renamed to websock

* removed stream from logs

* renamed ws to websock

* add connection closing test to common transport

* close incoming connection on ws stop

* renamed testwebsocket.nim -> testwstransport.nim

* removed raise todo

* split out/in connections

* add wss to tests

* Fix tls (#608)

* change log level

* fixed issue related to stopping

some cosmetic cleanup

* use `allFutures` to stop/close things

Prevent potential race conditions when stopping two or more transports

* misc

* point websock to server-case-object branch

* interop test with go

* removed websock version specification

* add daemon -> native ws test

* fix & test closed read/write

* update readOnce, thanks jangko

Co-authored-by: Dmitriy Ryajov <dryajov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Roadmap
Q3 2021
Development

Successfully merging this pull request may close these issues.

None yet

2 participants