vapor / async-kit

Sugary extensions for the SwiftNIO library
MIT License
71 stars 25 forks source link

New and better-behaved shutdown methods for `EventLoopGroupConnectionPool` #64

Closed gwynne closed 4 years ago

gwynne commented 4 years ago

Care has been taken not to break any existing API, despite this resulting in the confusing but for-now unavoidable presence of three distinct methods for shutting down pools. Especial care in particular has been taken to ensure that code using the legacy .shutdown() method will correctly and safely interoperate with code using the new .shutdownGracefully(_:) and .syncShutdownGracefully() methods in all cases.

codecov-io commented 4 years ago

Codecov Report

Merging #64 into master will increase coverage by 0.14%. The diff coverage is 93.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   94.43%   94.58%   +0.14%     
==========================================
  Files          23       23              
  Lines        1043     1182     +139     
==========================================
+ Hits          985     1118     +133     
- Misses         58       64       +6     
Flag Coverage Δ
#unittests 94.58% <93.95%> (?)
Impacted Files Coverage Δ
Sources/AsyncKit/EventLoop/EventLoop+Future.swift 60.00% <0.00%> (-40.00%) :arrow_down:
...es/AsyncKit/EventLoopFuture/FutureExtensions.swift 73.33% <0.00%> (-26.67%) :arrow_down:
.../ConnectionPool/EventLoopGroupConnectionPool.swift 93.00% <93.33%> (-1.12%) :arrow_down:
Tests/AsyncKitTests/AsyncKitTests.swift 100.00% <100.00%> (ø)
Tests/AsyncKitTests/ConnectionPoolTests.swift 89.53% <100.00%> (+2.49%) :arrow_up:
...ests/AsyncKitTests/EventLoopFutureQueueTests.swift 98.61% <100.00%> (+0.02%) :arrow_up:
Tests/AsyncKitTests/EventLoopGroup+Future.swift 100.00% <100.00%> (ø)
Tests/AsyncKitTests/FlattenTests.swift 100.00% <100.00%> (ø)
Tests/AsyncKitTests/Future+CollectionTests.swift 100.00% <100.00%> (ø)
Tests/AsyncKitTests/Future+OptionalTests.swift 100.00% <100.00%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 48a719d...2725124. Read the comment docs.

gwynne commented 4 years ago

One question about CI, how come there is a "push" and "pull_request" run for each image?

This seems like its unnecessarily doubling the amount of test cases that need to run on each PR. Or am I missing something?

Search me. It was part of the configuration when I got here, I assumed originally it had something to do with making codecov work and left it alone. I would be delighted to yank the extra event if it's actually superfluous.

tanner0101 commented 4 years ago

Hmm. vapor/vapor only has pull_request: https://github.com/vapor/vapor/blob/master/.github/workflows/test.yml#L3

I think we added push as well so that code cov baseline would get updated when merging to master. Maybe we can just have push and not pull_request, too? Seems really extra to have each test run twice.

gwynne commented 4 years ago

I think we added push as well so that code cov baseline would get updated when merging to master. Maybe we can just have push and not pull_request, too? Seems really extra to have each test run twice.

If the goal is to update the baseline for merges, there's at least one better event for that. I'll do a quick poke through the reference and find an appropriate one.

tanner0101 commented 4 years ago

These changes are now available in 1.1.0