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

Remove use of NIOAtomics in favor of Atomics #120

Closed MahdiBM closed 2 years ago

MahdiBM commented 2 years ago

The Problem

Starting with SwiftNIO 2.41.0, NIOAtomics is deprecated in favor of Atomics. This produces a warning if you use SwiftNIO 2.41.0 or higher.

Modifications

This PR add a dependency to Atomics to change the only use of NIOAtomics to Atomics, and suppress the warning.

What Warning?!

path/to/package/.build/checkouts/websocket-kit/Sources/WebSocketKit/WebSocketClient.swift:40:22: warning: 'NIOAtomic' is deprecated: please use ManagedAtomic from https://github.com/apple/swift-atomics instead
    let isShutdown = NIOAtomic.makeAtomic(value: false)
                     ^
codecov-commenter commented 2 years ago

Codecov Report

Merging #120 (4a3781a) into main (efb1bd5) will decrease coverage by 7.40%. The diff coverage is 14.28%.

@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
- Coverage   73.95%   66.55%   -7.41%     
==========================================
  Files           5        6       +1     
  Lines         503      601      +98     
==========================================
+ Hits          372      400      +28     
- Misses        131      201      +70     
Flag Coverage Δ
unittests 66.55% <14.28%> (-7.41%) :arrow_down:

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

Impacted Files Coverage Δ
Sources/WebSocketKit/WebSocketClient.swift 76.80% <14.28%> (-2.54%) :arrow_down:
...bSocketKit/Concurrency/WebSocket+Concurrency.swift 29.78% <0.00%> (ø)
0xTim commented 2 years ago

@gwynne looks like nightlies are failing in the CI scripts

0xTim commented 2 years ago

Going to merge this - the build failure is a CI issue rather than something introduced by the PR

VaporBot commented 2 years ago

These changes are now available in 2.6.1

JetForMe commented 2 years ago

@0xTim Heads up, this PR sets the compareExchange ordering to .relaxed, which is different from how the old NIOAtomics does compareAndExchange (iiuc).

MahdiBM commented 2 years ago

@JetForMe That should not be a problem. I had seen NIO's own code default to .relaxed, that's why I used .relaxed. Digging into it further, I see NIO also has used .relaxed in their own NIOAtomics to Atomics conversion: https://github.com/apple/swift-nio/blame/main/Sources/NIOPosix/PendingWritesManager.swift#L319

0xTim commented 2 years ago

Yep that's why I approved it

JetForMe commented 2 years ago

I don't really understand the nuances of the different orderings, I was just lucky to find this sentence in the comments: "Additionally, it uses a sequentially consistent ordering."