Add tests for listener Close + invoke only pool release
#250
Conversation
brandur
force-pushed
the
brandur-pgxv5-listener-close-test
branch
2 times, most recently
from
2219b73
to
b0eb5d3
Compare
| return c.closeFunc() | ||
| } | ||
|
|
||
| func testPoolConfig() *pgxpool.Config { |
There was a problem hiding this comment.
Maybe riverinternaltest should be its own package so we can reuse this stuff, but ugh, skipped any further refactoring there for now.
brandur
force-pushed
the
brandur-pgxv5-listener-close-test
branch
from
b0eb5d3
to
84fa0e8
Compare
| err := l.conn.Conn().Close(ctx) | ||
|
|
||
| // Regardless of the error state returned above, always release and unset | ||
| // the listener's local connection. |
There was a problem hiding this comment.
I'm not actually sure this is a good idea and I don't think we want to return the conn to the pool. If there are LISTEN subscriptions active which weren't properly UNLISTEN'd, the conn may still receive additional notifications that could interfere with other operations on it.
For pgbouncer compatibility reasons, we will probably also end up going a direction where the listener conn is created from a standalone config or at least a separate pool from the main river db pool.
Or maybe you can elaborate more on why you think this is a necessary change?
There was a problem hiding this comment.
Okay, fair point on that one, I forgot about the potential taint from not correctly using Unlisten. I put the Close back in and added a comment to remind us why it's written this way. One of the test cases had to go because pgx's implementation is very specific, and AfterRelease doesn't get called if a connection was released that was already closed.
Want to take another look?
| return c.closeFunc() | ||
| } | ||
|
|
||
| func testPoolConfig() *pgxpool.Config { |
Here, follow up #246 by adding a few more tests that verify a listener's state after `Close` has been invoked, including if it returned an error, which we're able to simulate by overriding pgx's `DialFunc` and returning a stand-in stub for an underlying `net.Conn`. Also, remove the explicit `Close` call on an underlying connection in favor of just invoking the pool's `Release` function. In case of an error condition, `Release` will detect that and do the right thing, and pgx is better tested/vetted to make sure that right thing happens.
brandur
force-pushed
the
brandur-pgxv5-listener-close-test
branch
from
84fa0e8
to
fb1e2b0
Compare
|
ty. |
Here, follow up #246 by adding a few more tests that verify a listener's
state after
Closehas been invoked, including if it returned an error,which we're able to simulate by overriding pgx's
DialFuncandreturning a stand-in stub for an underlying
net.Conn.Also, remove the explicit
Closecall on an underlying connection infavor of just invoking the pool's
Releasefunction. In case of anerror condition,
Releasewill detect that and do the right thing, andpgx is better tested/vetted to make sure that right thing happens.