Skip to content
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

Adds function ExchangeWithConn #1110

Merged
merged 4 commits into from May 4, 2020
Merged

Adds function ExchangeWithConn #1110

merged 4 commits into from May 4, 2020

Conversation

mostfunkyduck
Copy link
Contributor

#614 requested that connection reuse be implemented in this library to help improve performance when using TCP/TCP-TLS connections. This was rejected, fairly, because of the complexity in handling that logic within the library. This PR would solve the problem without introducing connection management code into the library at all. Instead it does some minor refactoring in order to expose the ability for the caller to manage connections and pass them in to the library via the ExchangeWithConn function. The existing Exchange behavior is preserved, so backwards compatibility shouldn't be an issue.

…n a connection instead of having the library create a new one for them. Exchange now wraps around this, implementing the existing behavior by creating a new connection and passing it to ExchangeWithConn. c.exchange has been updated to support this behavior as well.
@codecov-io
Copy link

codecov-io commented May 3, 2020

Codecov Report

Merging #1110 into master will increase coverage by 0.00%.
The diff coverage is 77.77%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1110   +/-   ##
=======================================
  Coverage   55.59%   55.60%           
=======================================
  Files          41       41           
  Lines       10067    10068    +1     
=======================================
+ Hits         5597     5598    +1     
  Misses       3441     3441           
  Partials     1029     1029           
Impacted Files Coverage Δ
client.go 70.05% <77.77%> (+0.15%) ⬆️

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 d128d10...e811691. Read the comment docs.

@miekg
Copy link
Owner

miekg commented May 4, 2020

this looks good, thank you

/lgtm

cbot[bot]
cbot bot approved these changes May 4, 2020
Copy link

@cbot cbot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by miekg

@miekg miekg merged commit 1fc9fa1 into miekg:master May 4, 2020
aanm pushed a commit to cilium/dns that referenced this pull request Jul 29, 2022
* Implements ExchangeWithConn, a function that allows callers to pass in a connection instead of having the library create a new one for them.  Exchange now wraps around this, implementing the existing behavior by creating a new connection and passing it to ExchangeWithConn.  c.exchange has been updated to support this behavior as well.

* adding tab

* formatting problem

* adds test case for ExchangeWithConn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants