pool.end() resolves before the last pool.query()

[asadbek-raimov]

Here is the link to the issue: 2163

[brianc]

Hey - this is awesome, thank you! Sorry for the delay - I'm in the mountains in colorado until Monday so i'm a bit slow to respond. Will be a lot more avail when I get home...but thanks so much for fixing/improving this. been a long-standing little bug...nice to get it taken care of! ❤️

[asadbek-raimov]

Hi! I should add some corrections though, now two other tests seem not passing so I will fix it

[brianc]

Hi! I should add some corrections though, now two other tests seem not passing so I will fix it

Howdy - looks like one of the tests for idle timeout still isn't passing - you still lookin' into that?

[asadbek-raimov]

Hi! I should add some corrections though, now two other tests seem not passing so I will fix it

Howdy - looks like one of the tests for idle timeout still isn't passing - you still lookin' into that?

Yeah, I am trying to figure out what's wrong with that

[hjr3]

I believe the idle timeout is failing due to a new data race caused by this fix. We now emit on remove when client ends:

const context = this
client.end(() => {
  context.emit('remove', client)
  ...
})

but the client may not end right away. Our test releases both clients and immediately sets up an "on remove" listener called removal. The intention is for removal to handle the removes caused by the idle timeout. However, removal will now sometimes handle the remove from release.

Log:

• clientB.release(new Error()) is called by the test source
  • _remove is called, but client.end is still pending and has not called the callback source
  • idle timeout is set source
  • the client is pushed into the idle queue source
• removal handler is set up source
• client.end is complete and now calls the callback source
• removal handle asserts that idle count is 0, but it is still 1 source

I am not sure of the exact fix at the moment, but I think we need a way to wait for client release to finish.

[hjr3]

I am not sure of the exact fix at the moment, but I think we need a way to wait for client release to finish.

waiting will not work because we are trying to catch the remove event that will fix in 10ms after clientA.release() is called

the attempted fix of counting clients will not work consistently because the order in which clientA and clientB are removed is non-deterministic>

in https://github.com/brianc/node-postgres/pull/3461 i check for clientA in the on remove callback, which i believe is the original intent of this test.

[asadbek-raimov]

@hjr3 Hi! Thanks for information, I will take a look