-
-
Notifications
You must be signed in to change notification settings - Fork 25
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
Heavily revise the implementation of EventLoopConnectionPool #102
Conversation
…work on a much more advanced replacement,
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 Report
Additional details and impacted files@@ 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
|
Fix a migration missing an important deletion in its revert that was revealed by vapor/async-kit#102
Holding off on merging this until @0xTim can glance at it too, given how fundamental this awful code is to Fluent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - one query but nothing major
These changes are now available in 1.18.0 |
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
Deque
instead of the obsoleteCircularBuffer
for tracking available connectionsOrderedDictionary
and numeric waiter IDs to track waitlists rather than the obsoleteCircularBuffer