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

Add connection pool queuing strategies in HostClient. #1238

Merged
merged 1 commit into from Mar 15, 2022

Conversation

u5surf
Copy link
Contributor

@u5surf u5surf commented Mar 5, 2022

Fixes #1236

@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Mar 7, 2022

I'm curios, have you tested if FIFO results in a better distribution in your case?

@u5surf
Copy link
Contributor Author

u5surf commented Mar 7, 2022

Hi, @erikdubbelboer
I'm not sure FIFO can be better for fairness of using their connections.
@bssd7 Can you test for this patch on your environment to resolve your problem?

@bssd7
Copy link

bssd7 commented Mar 8, 2022

i will check it

@bssd7
Copy link

bssd7 commented Mar 8, 2022

How can i add this fork into my go.mod file?
When i try to pull it i get following error:

go get github.com/u5surf/fasthttp@issue-1236
go get: github.com/u5surf/fasthttp@v1.33.1-0.20220307014346-46010ca954f2: parsing go.mod:
module declares its path as: github.com/valyala/fasthttp
but was required as: github.com/u5surf/fasthttp

@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Mar 8, 2022

It should work if you add this to your go.mod file:

replace (
    github.com/valyala/fasthttp => github.com/u5surf/fasthttp v1.33.1-0.20220307014346-46010ca954f2
)

@bssd7
Copy link

bssd7 commented Mar 11, 2022

Hello,

the load balancing is much better now and is working fine see pictures below.

load balancing after nginx:
image

load balancing after fasthttp: FIFO
image

old load balancing after fast http before change: LIFO
image

As you can see the load balancing is much smoother now. The differences in the load balancing which are left are properly an issue from the difference in the connection pool minimum size. Nginx and other http clients are working with a pre allocated pool minimum size and fasthttp does not support a pool minimum size.

Thank you, great work.

@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Mar 13, 2022

That is great. The last thing I'm wondering now is if we should maybe make this the default. It seems like this is an issue most people won't be able to recognize and change the configurations. And I don't really see how it could break someone's current use case. So I think it's best if we make this the default, can you change that?

@u5surf
Copy link
Contributor Author

u5surf commented Mar 13, 2022

@erikdubbelboer @bssd7 Of course, if maintainers agree with making this default, I'll change such that. Thanks all!

client.go Outdated
cc = c.conns[0]
c.conns[0] = nil
c.conns = c.conns[1:]
Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 14, 2022

Choose a reason for hiding this comment

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

Suggested change
cc = c.conns[0]
c.conns[0] = nil
c.conns = c.conns[1:]
cc = c.conns[0]
copy(c.conns, c.conns[1:])
c.conns[n] = nil
c.conns = c.conns[:n]

Otherwise c.conns = c.conns[1:] causes an allocation of a new backing slice.

Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 14, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@u5surf u5surf Mar 14, 2022

Choose a reason for hiding this comment

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

@erikdubbelboer
Thanks reviewing! Your suggestion which use copy without a new allocation is completely right indeed, But your suggested code has failed following test on my environment.

--- PASS: TestHostClientMaxConnWaitTimeoutError (0.01s)
=== CONT  TestHostClientConcurrent
panic: runtime error: index out of range [1] with length 1

goroutine 594 [running]:
github.com/valyala/fasthttp.(*HostClient).acquireConn(0xc0005891e0, 0x0, 0x0)
        /home/u5surf/fasthttp/client.go:1571 +0xd45
github.com/valyala/fasthttp.(*HostClient).doNonNilReqResp(0xc0005891e0, 0xc00012e380, 0xc000023800)
        /home/u5surf/fasthttp/client.go:1417 +0x5b2
github.com/valyala/fasthttp.(*HostClient).do(0x1f, 0x20, 0xc000023800)
        /home/u5surf/fasthttp/client.go:1368 +0xc9
github.com/valyala/fasthttp.(*HostClient).Do(0xc0005891e0, 0xc00012e380, 0xc0003064a0)
        /home/u5surf/fasthttp/client.go:1315 +0x114
github.com/valyala/fasthttp.doRequestFollowRedirects(0xc00012e380, 0xc000023800, {0xc0003064a0, 0x1f}, 0x10, {0xc06480, 0xc0005891e0})
        /home/u5surf/fasthttp/client.go:1034 +0x1da
github.com/valyala/fasthttp.doRequestFollowRedirectsBuffer(0xf85100, {0xc000306460, 0x1f, 0x20}, {0xc0003064a0, 0x1f}, {0xc06480, 0xc0005891e0})
        /home/u5surf/fasthttp/client.go:1015 +0x2a5
github.com/valyala/fasthttp.clientGetURL({0xc000306460, 0x1f, 0x20}, {0xc0003064a0, 0x1f}, {0xc06480, 0xc0005891e0})
        /home/u5surf/fasthttp/client.go:886 +0x133
github.com/valyala/fasthttp.(*HostClient).Get(0xb2fb99, {0xc000306460, 0x1f, 0x20}, {0xc0003064a0, 0x1f})
        /home/u5surf/fasthttp/client.go:838 +0x88
github.com/valyala/fasthttp.testClientGet(0xc000224680, {0xc064a0, 0xc0005891e0}, {0xb30193, 0x11}, 0x1e)
        /home/u5surf/fasthttp/client_test.go:2326 +0x175
github.com/valyala/fasthttp.testHostClientGet(...)
        /home/u5surf/fasthttp/client_test.go:2410
github.com/valyala/fasthttp.TestHostClientConcurrent.func1()
        /home/u5surf/fasthttp/client_test.go:2315 +0xd3
created by github.com/valyala/fasthttp.TestHostClientConcurrent
        /home/u5surf/fasthttp/client_test.go:2313 +0x145
FAIL    github.com/valyala/fasthttp     1.046s

my go version

 go version
go version go1.17.8 linux/amd64

Thus, I just modified following using copy, then passed all tests.

@@ -1568,7 +1568,8 @@ func (c *HostClient) acquireConn(reqTimeout time.Duration, connectionClose bool)
                case FIFO:
                        cc = c.conns[0]
                        c.conns[0] = nil
-                       c.conns = c.conns[1:]
+                       copy(c.conns, c.conns[1:])
+                       c.conns = c.conns[:len(c.conns)-1]
                default:
                        return nil, ErrConnPoolStrategyNotImpl
                }

As it relates to this patch, I found a curious article about performance of FIFO implementation with copy.
https://www.reddit.com/r/golang/comments/24dfn8/someones_right_building_a_queue_list_or_slices/ch62raz/?context=8&depth=9

Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 14, 2022

Choose a reason for hiding this comment

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

My bad, it should have been n-1. You already have the length, no need to call len again.

client.go Outdated
// Connection pool strategy determines whether LIFO or FIFO storategy for connection pool
ConnPoolStrategy ConnPoolStrategyType
Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 14, 2022

Choose a reason for hiding this comment

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

Suggested change
// Connection pool strategy determines whether LIFO or FIFO storategy for connection pool
ConnPoolStrategy ConnPoolStrategyType
// Connection pool strategy. Can be either LIFO or FIFO (default).
ConnPoolStrategy ConnPoolStrategyType

client.go Outdated
@@ -1497,6 +1508,10 @@ var (
// to broken server.
ErrConnectionClosed = errors.New("the server closed connection before returning the first response byte. " +
"Make sure the server returns 'Connection: close' response header before closing the connection")

// ErrConnPoolStrategyNotImpl is returned when HostClient.ConnPoolStrategy is not implement yet.
// If you see this error, then you need to check HostClient configuration once.
Copy link
Collaborator

@erikdubbelboer erikdubbelboer Mar 14, 2022

Choose a reason for hiding this comment

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

Suggested change
// If you see this error, then you need to check HostClient configuration once.
// If you see this error, then you need to check your HostClient configuration.

* At first, we implement LIFO(conventional) and FIFO which indicate in valyala#1236
* Change default strategy FIFO
@erikdubbelboer erikdubbelboer merged commit 8f5e51f into valyala:master Mar 15, 2022
11 checks passed
@erikdubbelboer
Copy link
Collaborator

erikdubbelboer commented Mar 15, 2022

Thanks for building this!

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.

3 participants