Confusing error message: detected infinite loop

[ajeetdsouza]

Hi, thanks for maintaining pgx! I was testing some Go code that was reliant on pgxpool with Antithesis, and ran into the following error:

pgxpool: detected infinite loop acquiring connection; likely bug in PrepareConn or BeforeAcquire hook

This was confusing, because I hadn't set a PrepareConn / BeforeAcquire hook, but I went through the code and found this:

pgx/pgxpool/pool.go

Lines 630 to 634 in 4c1308c

	err := cr.conn.Ping(pingCtx)
	if err != nil {
	res.Destroy()
	continue
	}

It seems that in the event of a network error, if all pings to Postgres fail, pgxpool throws a detected infinite loop error. Could the error message here be improved somehow - and would it be possible to use a sentinel error that can be caught and handled?

[jackc]

Hmm... I wouldn't expect a network error to trigger that code path. What I would expect to happen is for the ping or PrepareConn hook to find all connections in the pool dead and then the last Acquire to fail when creating the new connection. Then that error would be returned.

Do you have any way of duplicating this situation?

[ajeetdsouza]

Antithesis is a chaos testing tool that checks a lot of code paths - what I'm saying is that while it may be unlikely, there is definitely a logical code path in which no PrepareConn or BeforeAcquire hook has been set up, but pgxpool still flags it as an error:

• Pings on all existing Postgres connections fail
• Acquire manages to acquire a new connection to Postgres
• Due to a fault, pinging the new Postgres instance also fails

Would you accept a PR to:

1. Clarify the error message to account for this scenario
2. Create a sentinel error that can be caught in downstream packages - the current code uses errors.New(), and the only way to catch this error is to match on the text of the error string.

[jackc]

Antithesis is a chaos testing tool that checks a lot of code paths - what I'm saying is that while it may be unlikely, there is definitely a logical code path in which no PrepareConn or BeforeAcquire hook has been set up, but pgxpool still flags it as an error:

* Pings on all existing Postgres connections fail

* Acquire manages to acquire a new connection to Postgres

* Due to a fault, pinging the new Postgres instance also fails

Based on my review of the code, it is extremely unlikely this would trigger. There would need to be a custom ShouldPing function that was calling ping unnecessarily.

Would you accept a PR to:

1. Clarify the error message to account for this scenario

I guess we could change detected infinite loop acquiring connection to something like too many failed attempts acquiring connection. And I suppose likely bug in PrepareConn or BeforeAcquire hook could change to likely bug in PrepareConn, BeforeAcquire, or ShouldPing

2. Create a sentinel error that can be caught in downstream packages - the current code uses `errors.New()`, and the only way to catch this error is to match on the text of the error string.

I would actually need a little more convincing on this change. It's only worth while making it easy for application code to match a particular error if the application will behave differently based on the type of the error. Otherwise, we are adding to the public interface of the package for no benefit. And given that by far the most like cause of this error occurring is a bug in PrepareConn, BeforeAcquire, or ShouldPing, there is no reasonable application run time behavior change. The correct response is to fix the underlying bug.

[ajeetdsouza]

I guess we could change detected infinite loop acquiring connection to something like too many failed attempts acquiring connection. And I suppose likely bug in PrepareConn or BeforeAcquire hook could change to likely bug in PrepareConn, BeforeAcquire, or ShouldPing

This would definitely be a good change to have.

it is extremely unlikely this would trigger

And given that by far the most like cause of this error occurring is a bug in PrepareConn, BeforeAcquire, or ShouldPing, there is no reasonable application run time behavior change.

unlikely != impossible here - there is a logical path where one runs into this error without setting up any hooks, which I appear to have run into during chaos testing. I was hoping to handle it by exiting with the same exit code that I use when Postgres is down and a connection cannot be established to it, but I have to use string matching on the error message because there is no sentinel.

[jackc]

I updated the error message as described above.

unlikely != impossible here - there is a logical path where one runs into this error without setting up any hooks, which I appear to have run into during chaos testing.

The intent of the loop is to drain the pool of broken connections if a network interruption occurs. Once the pool is drained, new connection attempts will be made. Either it will succeed and all is well, or it will fail and the connection error will be returned. The intent of the limit on loop iterations is as a fail-safe in case of some sort of infinite loop bug (presumably caused by a buggy PrepareConn).

So if it's triggering outside of that there is a problem. But I just spent a while walking through this code path and I don't see any way it could occur outside of extremely contrived scenarios.

For example:

1. Multiple concurrent acquirers
2. Connection attempts always succeed
3. There is some sort of timeout that kills the underlying connection
4. One unlucky acquirer always gets recycled connections that need to call the ping when the underlying connection is dead.

We could increase the number of allowed attempts to make it even more vanishingly unlikely to happen in the real world, but I'm not sure it's really worth it.