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

plugin/forward: make Yield not block #3336

Merged
merged 2 commits into from Oct 1, 2019

Conversation

@miekg
Copy link
Member

miekg commented Oct 1, 2019

Yield may block when we're super busy with creating (and looking) for
connection. Set a small timeout on Yield, to skip putting the connecting
back in the queue.

Use persistentConn troughout the socket handling code to be more
consistent.

@miekg miekg changed the title plugin/forward: may Yield not block plugin/forward: make Yield not block Oct 1, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 1, 2019

Codecov Report

Merging #3336 into master will decrease coverage by <.01%.
The diff coverage is 79.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3336      +/-   ##
==========================================
- Coverage   55.19%   55.18%   -0.01%     
==========================================
  Files         216      216              
  Lines       10774    10772       -2     
==========================================
- Hits         5947     5945       -2     
- Misses       4368     4369       +1     
+ Partials      459      458       -1
Impacted Files Coverage Δ
plugin/forward/setup.go 56.83% <100%> (-0.31%) ⬇️
plugin/forward/proxy.go 86.66% <100%> (ø) ⬆️
plugin/forward/connect.go 81.94% <77.77%> (+0.25%) ⬆️
plugin/forward/persistent.go 95.65% <78.57%> (-2.94%) ⬇️
plugin/file/reload.go 75% <0%> (+5.55%) ⬆️

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 7b69dfe...52241bb. Read the comment docs.

plugin/forward/connect.go Outdated Show resolved Hide resolved
@@ -82,54 +82,6 @@ func TestCleanupByTimer(t *testing.T) {
tr.Yield(c4)
}

func TestPartialCleanup(t *testing.T) {

This comment has been minimized.

Copy link
@rdrozhdzh

rdrozhdzh Oct 1, 2019

Contributor

do you plan to keep this test?

This comment has been minimized.

Copy link
@miekg

miekg Oct 1, 2019

Author Member

no, it was already not ideal with all those sleeps

@miekg miekg force-pushed the test branch from 3180062 to 9ef3328 Oct 1, 2019
@miekg

This comment has been minimized.

Copy link
Member Author

miekg commented Oct 1, 2019

@rdrozhdzh

This comment has been minimized.

Copy link
Contributor

rdrozhdzh commented Oct 1, 2019

I want to drop conns created, not used, X seconds ago.

Hm, which benefits are you going to achieve with this new approach? I think existing implementation uses cached connections more efficiently.

@rdrozhdzh

This comment has been minimized.

Copy link
Contributor

rdrozhdzh commented Oct 1, 2019

Also, the connManager() code relies on the fact that all connections in cache are ordered by used. With your new approach this logic will get broken.

@miekg

This comment has been minimized.

Copy link
Member Author

miekg commented Oct 1, 2019

@miekg

This comment has been minimized.

Copy link
Member Author

miekg commented Oct 1, 2019

Yield may block when we're super busy with creating (and looking) for
connection. Set a small timeout on Yield, to skip putting the connection
back in the queue.

Use persistentConn troughout the socket handling code to be more
consistent.

Signed-off-by: Miek Gieben <miek@miek.nl>

Dont do

Signed-off-by: Miek Gieben <miek@miek.nl>
@miekg miekg force-pushed the test branch from 9ef3328 to 52241bb Oct 1, 2019
@rdrozhdzh

This comment has been minimized.

Copy link
Contributor

rdrozhdzh commented Oct 1, 2019

In Yield() we add connection to the end of slice. Since we always updated the used field with Now() the connections were automatically ordered by used
In Dial() we take the last (the freshest) connection. If it appeared expired then we assume all connections before it are also expired. And we call cleanup for all those connections.

In you new approach, when used connection gets back to cache its created field may not be the greatest among other cached connection. So, cached connections will get unordered and at Dial time we'll have to check each connection if it's expired or not.

@miekg

This comment has been minimized.

Copy link
Member Author

miekg commented Oct 1, 2019

if c != nil {
return c, true, nil
if pc != nil {
pc.used = time.Now() // update used time

This comment has been minimized.

Copy link
@rdrozhdzh

rdrozhdzh Oct 1, 2019

Contributor

If we update used at dial time this doesn't guarantee that connections will be yielded in the same order. Ideally, connManager() should (and actually does) update time at yield time. As for me, I would keep the existing logic.
But I guess you want to move as much code as possible from connManager() loop. That's why I suggested to update used in func (t *Transport) Yield(pc *persistConn). This could make cached connections slightly unordered, though.

This gives one central place where we update used in the persistConns

Signed-off-by: Miek Gieben <miek@miek.nl>
@miekg

This comment has been minimized.

Copy link
Member Author

miekg commented Oct 1, 2019

Copy link
Contributor

rdrozhdzh left a comment

lgtm

@miekg miekg merged commit 2d98d52 into master Oct 1, 2019
4 checks passed
4 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
@corbot corbot bot deleted the test branch Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.