More robust listener close in riverpgxv5

[brandur]

As part of #239, we're observing the possibility of the notifier going
into a hot loop where it's trying to reopen a listener connection, but
the listener won't let it because it thinks the connection is already
open.

A suspect is the listener's Close implementation, which in the event
of an error, returns the error and fails to release an underlying
connection, putting it into a state where it's never reusable.

Here, modify Close so that it always releases and unsets an underlying
connection regardless of the error state returned.

I tried to add a test case for this, but reading through pgx and net
source code, I couldn't find any way to simulate an error from Close
(I thought a cancelled context would do it, but it does not), so I had
to leave it.

Thanks @mfrister for pointing this one out!

Fixes #248.

[brandur]

@bgentry Seriously tried to add a test case for this, but couldn't find a way to make Close on the PG conn return an error. The only way to do it I think would be to add additional test-only interface functions to inject state, which would be kind of gross. Thoughts?

[mfrister]

Thanks for the fix!

I've deployed a version of my worker with riverpgxv5 from this branch and will report on Monday whether the issue appeared again. So far it looks good.

Enjoy your weekend!

[Rshep3087]

There is this package from pgx https://github.com/jackc/pgx/blob/master/pgxtest/pgxtest.go. You can overwrite the CloseFunc on the test runner I believe.

[brandur]

@mfrister Excellent. Thanks!

@Rshep3087 Hm, just reading through this module, I believe ConnTestRunner is more of a helper for controlling the flow of test runs rather than something that can act as a mock for a pgx.Conn.

[bgentry]

The only ways I can think of to test this are (a) use an interface over the conn here, which feels excessive, or (b) somehow force the server to uncleanly terminate the conn or reply with an error upon close.

I'm fine with not going forward with one of those approaches at this time, they seem to heavy for the benefit at this stage. Thanks for the quick fix

[bgentry]

Man, I noticed this change when I reviewed the mega-PR, should have said something 😞

[brandur]

Doh. Well, I was going to say "next time", but hopefully nothing like that PR ever happens again :)

[brandur]

The only ways I can think of to test this are (a) use an interface over the conn here, which feels excessive, or (b) somehow force the server to uncleanly terminate the conn or reply with an error upon close.

Yeah, I had the same reactions.

I think (a) is plausible, but has the downside that it makes the code a little more painful to use. Rather than being able to easily jump easily into concrete implementations, you now have to jump through the interface, which is always a bit annoying.

And RE: (b), I read through a lot of pgx source around Close, and man, as far as I can tell, it is quite difficult to ever get it to return an error. The underlying net.Conn under the pgx connection which is under the pgx pool connection will under some cases, but I couldn't find an obvious way to trigger it, especially from this level.

I'm fine with not going forward with one of those approaches at this time, they seem to heavy for the benefit at this stage. Thanks for the quick fix

Sweet. Yeah, that was my conclusion as well. Possible maybe to add some form of testing, but maybe more trouble than it's worth, and getting the fix out the door is the best compromise for the time being.

Thanks!

[dhermes]

Regarding unit testing, I have a little bit of experience here. I hunted down a bug in lib/pq a few years back and ended up replicating the bug locally while crafting a fix by using a TCP proxy that would send an RST packet after some signal had been received (but before the actual PostgreSQL connection was closed).

From what I understand, your unit tests hit a REAL PostgreSQL instance, so you could run a proxy within a Go unit test and put that proxy in front of the PostgreSQL instance available to the unit tests.

🤞 I'd hope a plain old TCP error over a non-TLS connection would do the trick, but maybe not. In my log capture around the issue that this fixed (#248), it was actually a TLS protocol issue that showed up in the error:

{
    "level": "error",
    "time": "2024-03-01T23:56:08.679126682Z",
    "notifier": {
        "err": {
            "error": "tls: failed to send closeNotify alert (but connection was closed anyway): write tcp 10.122.48.181:44034->10.122.30.240:5432: i/o timeout",
            "kind": "*fmt.wrapError",
            "stack": null
        }
    },
    "subsystem": "river",
    "message": "error closing listener"
}

However, if I had to guess I'd imagine the TCP connection is what had already been closed (e.g. via RST).

[brandur]

@dhermes Thanks for the added detail there! I'm sort of hoping to avoid a full-blown proxy for the time being if we can possibly do it (looked at yours — looks good, but definitely a substantial amount of new code). You gave me the idea though of getting something more like an "in code" proxy going, and I found a fairly low touch way of simulating a Close error using a combination of pgxpool's DialFunc and a lightweight net.Conn mock. See #250.

type connStub struct {
	net.Conn

	closeFunc func() error
}

func newConnStub(conn net.Conn) *connStub {
	return &connStub{
		Conn: conn,

		closeFunc: conn.Close,
	}
}

func (c *connStub) Close() error {
	return c.closeFunc()
}

		var connStub *connStub

		config := testPoolConfig()
		config.ConnConfig.DialFunc = func(ctx context.Context, network, addr string) (net.Conn, error) {
			// Dialer settings come from pgx's default internal one (not exported unfortunately).
			conn, err := (&net.Dialer{KeepAlive: 5 * time.Minute}).Dial(network, addr)
			if err != nil {
				return nil, err
			}

			connStub = newConnStub(conn)
			return connStub, nil
		}

[mfrister]

As promised above:

I've deployed a version of my worker with riverpgxv5 from this branch and will report on Monday whether the issue appeared again. So far it looks good.

The problem hasn't reappeared on my worker over the weekend, as expected.

[brandur]

@mfrister That's great to hear!! Thanks for checking in about it.