Add connection pool queuing strategies in HostClient.

[u5surf]

Fixes #1236

• At first, we implement LIFO(conventional) and FIFO which indicate in Configurable LIFO, FIFO client connection pooling #1236

[erikdubbelboer]

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

[u5surf]

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]

i will check it

[bssd7]

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]

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]

Hello,

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

load balancing after nginx:

load balancing after fasthttp: FIFO

old load balancing after fast http before change: LIFO

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]

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]

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

[erikdubbelboer]

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.

[erikdubbelboer]

See: https://go.dev/play/p/bcrFeRc1I-n

[u5surf]

@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

[erikdubbelboer]

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

[erikdubbelboer]

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

[erikdubbelboer]

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.

[erikdubbelboer]

Thanks for building this!