vapor / async-kit

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

Heavily revise the implementation of EventLoopConnectionPool #102

Closed gwynne closed 1 year ago

gwynne commented 1 year ago

Overview

This is a temporary stopgap measure to clean up some of the more extant issues with the existing implementation, pending the integration of a much more robust and advanced connection pool solution.

While technically there is no change in the public API surface, this is nonetheless being marked as semver-minor due to the significant changes in behavior. This PR has already revealed a query ordering bug in the Fluent benchmarks just by behaving more correctly.

This should hopefully fix the issue described in #101, but I haven't been able to locally reproduce the race condition despite being fairly sure as to its cause, so I can't verify.

More details

github-advanced-security[bot] commented 1 year ago

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

codecov-commenter commented 1 year ago

Codecov Report

Merging #102 (deda984) into main (4ee8c97) will decrease coverage by 0.07%. Report is 1 commits behind head on main. The diff coverage is 95.78%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #102 +/- ## ========================================== - Coverage 95.70% 95.63% -0.07% ========================================== Files 19 19 Lines 955 963 +8 ========================================== + Hits 914 921 +7 - Misses 41 42 +1 ``` | [Files Changed](https://app.codecov.io/gh/vapor/async-kit/pull/102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor) | Coverage Δ | | |---|---|---| | [...ncKit/ConnectionPool/EventLoopConnectionPool.swift](https://app.codecov.io/gh/vapor/async-kit/pull/102?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=vapor#diff-U291cmNlcy9Bc3luY0tpdC9Db25uZWN0aW9uUG9vbC9FdmVudExvb3BDb25uZWN0aW9uUG9vbC5zd2lmdA==) | `94.85% <95.78%> (-0.46%)` | :arrow_down: |
gwynne commented 1 year ago

Holding off on merging this until @0xTim can glance at it too, given how fundamental this awful code is to Fluent.

penny-for-vapor[bot] commented 1 year ago

These changes are now available in 1.18.0