vapor / websocket-kit

WebSocket client library built on SwiftNIO
https://docs.vapor.codes/4.0/advanced/websockets/
MIT License
272 stars 79 forks source link

Make sure onPing is called and account for control frames being able to be interspersed with message fragments #114

Closed tkrajacic closed 2 years ago

tkrajacic commented 2 years ago

Control frames can appear in the middle of fragmented messages. Therefor they must not be part of the frameSequence. Also expose the frame data of the control frames, as that might be usable application data.

Before this PR, onPing was never called at all.

codecov-commenter commented 2 years ago

Codecov Report

Merging #114 (6f5ef1f) into main (e0faccd) will decrease coverage by 6.56%. The diff coverage is 85.00%.

:exclamation: Current head 6f5ef1f differs from pull request most recent head 9386a24. Consider uploading reports for the commit 9386a24 to get more accurate results

@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
- Coverage   71.15%   64.59%   -6.57%     
==========================================
  Files           5        6       +1     
  Lines         475      579     +104     
==========================================
+ Hits          338      374      +36     
- Misses        137      205      +68     
Flag Coverage Δ
unittests 64.59% <85.00%> (-6.57%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
Sources/WebSocketKit/WebSocket.swift 72.11% <85.00%> (+0.32%) :arrow_up:
...bSocketKit/Concurrency/WebSocket+Concurrency.swift 29.78% <0.00%> (ø)
tkrajacic commented 2 years ago

Come to think of it, this is not semver-patch, but actually changing public API. I guess I should split this up into two PRs. One that fixes the onPing not being called while also fixing the frameSequence bug, and the other just adding the bytebuffer for ping and pong callbacks?

0xTim commented 2 years ago

Correct, we can't just change the API as in the PR as it currently stands

tkrajacic commented 2 years ago

This should now be constrained to only internal fixes.

VaporBot commented 2 years ago

These changes are now available in 2.4.1