-
Notifications
You must be signed in to change notification settings - Fork 334
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Use synchronous I/O for sync methods. Fixes #62
- Loading branch information
Showing
9 changed files
with
253 additions
and
144 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
9f5ea21
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.
Here's a Proof of Concept that implements DNS Cache for
NETSTANDARD1_3
: caleblloyd@c647af5It feels hacky, but that's why it's limited to SyncIO on
NETSTANDARD1_3
. It's the only way I could think of that did not involve backporting the resolver fromNETSTANDARD2
.Let me know if you think this is a good idea to go forward with, and I can clean it up and open a PR to the
sync_io
branch.9f5ea21
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.
Thanks, I'll try to take a look at this later tonight.
9f5ea21
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.
The more I think about it, the more I dislike the DNS Cache option. Resolving hosts is a hard, multi-tier thing and making our own assumptions about caching could break other guarantees of the resolver (TTLs, round-robin responses, etc.)
I think the best approach is to implement
min pool size
in another commit/issue and suggest Connection Pool Warming for those who need high concurrency synchronous connections. We should probably start a "Best Practices" section in the README.md or on a wiki page:min pool size
in the connection string and callingdb.Connect(); db.Close();
before accepting any requestsThat being said, I think the
sync_io
branch looks great except we should probably add syncIO logic to the SemaphoreSlim calls in ConnectionPool.GetSessionAsync (example here)I'm getting consistently good Sync performance on a warmed connection pool. Connection pool can be warmed by running Async performance tests after a cold start, then running Sync performance tests.
9f5ea21
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.
I agree; I think this is best addressed as a "known issue" for now, with workarounds of using
OpenAsync
if possible and using a bigger minimum pool size (once that's implemented). Long term, netstandard-2.0 may provide more options.I want to add a few more tests (to verify the sync path) but after that I think it's worth merging to improve the library for sync callers.