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

Rework #5

Merged
merged 48 commits into from
Mar 18, 2021
Merged

Rework #5

merged 48 commits into from
Mar 18, 2021

Conversation

dryajov
Copy link
Member

@dryajov dryajov commented Mar 11, 2021

This is a minimal functional implementation that reworks:

  • API, it adds two methods to receive data
    • proc recv*(ws: WebSocket, data: pointer, size: int): Future[int] {.async.}, allows passing a pointer to up to size bytes, note that this will potentially consume multiple and/or partial frames. This will consume a frame as soon as it arrives, without buffering, which allows streaming.
    • proc recv*(ws: WebSocket, size = WSMaxMessageSize): Future[seq[byte]] {.async.} a convenience method that accumulates frames up to a max message size
    • Note that in WS messages split into arbitrary frames delimited by a fin flag
  • Fixes an important issue with framing, where a message would be stuffed into one frame, encoded and then split into chunks of 1MB, this is dangerous as anything else writing to the same pipe in between chunks will get interleaved with the frame chunks (this issue is also present in the nim-news package!), the correct behaviour is to split the data into smaller chunks and then frame that
  • Fixed control message handling
  • Fixed closing flow handling (NOTE, Websockets do not support half-closed/open flow)
  • General cleanup and testing

This PR does not address any of the HTTP handling functionality already present, which is largely based on the nim-json-rpc implementation but should potentially be reworked with the new HTTP functionality recently added to chronos.

@dryajov dryajov requested a review from arijitAD March 11, 2021 16:08
@dryajov dryajov requested a review from cheatfate March 12, 2021 00:13
src/ws.nim Show resolved Hide resolved
examples/client.nim Outdated Show resolved Hide resolved
src/ws.nim Outdated Show resolved Hide resolved
src/ws.nim Show resolved Hide resolved
src/ws.nim Show resolved Hide resolved
src/ws.nim Show resolved Hide resolved
Copy link

@sinkingsugar sinkingsugar left a comment

Choose a reason for hiding this comment

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

Well that was a rework indeed.
LGTM, maybe adding coverage tracking would help as it definitely needs lot of more testing to find runtime issues.

frame.opcode = (opcode).Opcode
if frame.length > 125:
raise newException(WSPayloadTooLarge,
"Control message payload is freater than 125 bytes!")
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: greater

@dryajov dryajov marked this pull request as ready for review March 16, 2021 17:05
@dryajov
Copy link
Member Author

dryajov commented Mar 16, 2021

This should be ready for review. As it is, it still lacks comprehensive tests, but it is a fully functional implementation with browser interop.

The next steps are:

  • HTTP rework and SSL support
  • Adding more comprehensive tests
  • Adding browser centric tests perhaps with karma or something similar
    • the most important point is that browser interop is guaranteed

@dryajov
Copy link
Member Author

dryajov commented Mar 16, 2021

CI is broken for i386 for github actions here is an issue tracking it - actions/runner-images#2919 (comment). The workaround didn't work, but it's not critical to merge.

@dryajov dryajov merged commit 3923a9b into main Mar 18, 2021
@jangko jangko deleted the rework branch May 20, 2021 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants