Cursor: avoid closing connection twice if error received after destroy()

[alxndrsn]

Fix for #1674, alternative to https://github.com/brianc/node-postgres/pull/2832.

[brianc]

Sorry for the delay here (things have been crazy over here recently). This looks good to me. If @charmander doesn't have any thoughts on the contrary I'm happy to merge & release this. Really appreciate your help & the test coverage!

[alxndrsn]

Thanks @brianc - all feedback very appreciated :slightly_smiling_face:

[alxndrsn]

@brianc @charmander is there anything I can do to improve this PR?

[alxndrsn]

Thanks @rafaell-lycan!

[nathanjcochran]

I am running into this issue as well (see my comment on #1674). I believe this PR would fix my problem. Would love to see this get merged!

[alxndrsn]

I believe this PR would fix my problem.

@nathanjcochran I see you have a neat repro at https://github.com/brianc/node-postgres/issues/1674#issuecomment-1563287449; any chance you can test this patch against that?

[nathanjcochran]

I see you have a neat repro at https://github.com/brianc/node-postgres/issues/1674#issuecomment-1563287449; any chance you can test this patch against that?

@alxndrsn Just tested it, and it does indeed fix the issue! 🎉

[alxndrsn]

@nathanjcochran Great news. Do you think your repro is significantly different from the test case in this PR? If so, should we consider turning it into an additional test case?

[nathanjcochran]

@alxndrsn It does seem somewhat different, in that my repro is attempting to cancel the query (which triggers an error in the database for that query - I'm not entirely sure if the tests in this PR do that?), close the stream, and reuse that exact same connection without first returning it to the pool (whereas the tests in this PR seem to return the connection to the pool first). So I do think it could make sense to add a separate test case. Perhaps something like this (didn't actually have a change to test this yet - I don't have everything set up properly to run the tests locally at the moment):

it('should work after cancelling query', async () => {
  const pool = new pg.Pool();
  const conn = await pool.connect();

  // Get connection PID for sake of pg_cancel_backend() call
  const result = await conn.query('SELECT pg_backend_pid() AS pid;');
  const { pid } = result.rows[0];

  const stream = conn.query(new QueryStream('SELECT pg_sleep(10);'));
  stream.on('data', (chunk) => {
    // Switches stream into readableFlowing === true mode
  });
  stream.on('error', (err) => {
    // Errors are expected due to pg_cancel_backend() call
  });

  // Create a promise that is resolved when the stream is closed
  const closed = new Promise((res) => {
    stream.on('close', res);
  });

  // Wait 100ms before cancelling the query
  await new Promise((res) => {
    setTimeout(() => res(), 100);
  });

  // Cancel pg_sleep(10) query
  await pool.query('SELECT pg_cancel_backend($1);', [pid]);

  // Destroy stream and wait for it to be closed
  stream.destroy();
  await closed;

  // Subsequent query on same connection should succeed
  const res = await conn.query('SELECT 1 AS a;');
  assert.deepStrictEqual(res.rows, [ { a:1 } ]);
  
  conn.release()
  await pool.end()
})

[alxndrsn]

Thanks @nathanjcochran - I'll give it a quick go. If the test is good, any objections to adding it to this PR?

[alxndrsn]

@nathanjcochran looks like the test is good: https://github.com/alxndrsn/node-postgres/actions?query=branch%3Adont-close-cursor-twice-new-test-test++

[alxndrsn]

I've rebased this branch onto the latest master. Prior to that I confirmed that tests were still failing without the fix in packages/pg-cursor/index.js.

[nathanjcochran]

If the test is good, any objections to adding it to this PR?

@alxndrsn Go for it! Thank you!!

[alxndrsn]

Thanks @nathanjcochran - added :+1:

[alxndrsn]

@brianc @charmander is there anything I can do to improve this PR? Very happy to discuss, or to close the PR if there is no interest in merging it.

[charmander]

I’d like to know why handleError is called after completion and haven’t looked into it yet.

(code style doesn’t match, by the way)

[alxndrsn]

@charmander I'm happy to update the code style if that will help. Do you have any pointers on what would need to change?

[lognaturel]

@charmander @brianc We've been running this in a fork for a year now and would really like to come back to mainline! Could one of you push the style changes that you want to this branch, maybe? 🙏

[alxndrsn]

@charmander I've updated the formatting to mirror surrounding code; is it correct now?

[alxndrsn]

I've rebased this branch onto master, and fixed all lint violations.

Interestingly two of the three new test cases now pass without the fix in this PR. From https://github.com/brianc/node-postgres/actions/runs/6586450194/job/17894838346?pr=2836:

  error recovery
    ✓ recovers from a streaming error in a transaction
    ✓ handles an error on a stream after a plain text non-stream error
    ✓ does not crash when closing a connection with a queued stream
    1) should work if used after timeout error
    ✓ should work if used after syntax error
    ✓ should work after cancelling query (112ms)

[alxndrsn]

I've merged the latest master into this branch and the tests pass :heavy_check_mark:

I've also run the tests against latest master without the fix, and there are two failures:

  2 failing
  1) error recovery
       should work if used after timeout error:
      AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual
- []
+ [
+   {
+     d: 4
+   }
+ ]
      + expected - actual
      -[]
      +[
      +  {
      +    "d": 4
      +  }
      +]
      
      at /home/runner/work/node-postgres/node-postgres/packages/pg-query-stream/test/error.ts:109:12
      at Generator.next (<anonymous>)
      at fulfilled (test/error.ts:5:58)
      at process._tickCallback (internal/process/next_tick.js:68:7)
  2) error recovery
       should work after cancelling query:
      AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual
- []
+ [
+   {
+     a: 1
+   }
+ ]
      + expected - actual
      -[]
      +[
      +  {
      +    "a": 1
      +  }
      +]
      
      at /home/runner/work/node-postgres/node-postgres/packages/pg-query-stream/test/error.ts:168:12
      at Generator.next (<anonymous>)
      at fulfilled (test/error.ts:5:58)
      at process._tickCallback (internal/process/next_tick.js:68:7)

In addition, there seems to be some kind of shutdown problem when the fix is not included.

See: https://github.com/alxndrsn/node-postgres/actions/runs/8277192701/job/22647097080

[brianc]

Thank you so much! Your help is amazing!

[alessbelli]

This fix looks amazing, thank you for getting that over the finish line! I don't know what the general policy is here for cutting new tags, but I wondered when this would be tagged and published to npm as a new version? there hasn't been a new version in a while so I'd like to know whether it's best for us to wait for 8.12 or to use this commit. Thanks again!

[brianc]

New version going out tomorrow! Typically doesnt take me this long…last day of my Job was today so gotta focus on spinning that down. Sorry for the delay. 🤪

On Fri, Mar 29, 2024 at 5:58 PM Alessandro Jeanteur < @.***> wrote:

This fix looks amazing, thank you for getting that over the finish line! I don't know what the general policy is here for cutting new tags, but I wondered when this would be tagged and published to npm as a new version? there hasn't been a new version in a while so I'd like to know whether it's best for us to wait for 8.12 or to use this commit. Thanks again!

— Reply to this email directly, view it on GitHub https://github.com/brianc/node-postgres/pull/2836#issuecomment-2027806203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAMHIMJMIJCSCHOAMJ43KLY2XW25AVCNFSM6AAAAAAQ63TZT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHAYDMMRQGM . You are receiving this because you modified the open/close state.Message ID: @.***>

[brianc]

New patch version out now! Sorry again for the delay

On Fri, Mar 29, 2024 at 9:33 PM Brian Carlson @.***> wrote:

New version going out tomorrow! Typically doesnt take me this long…last day of my Job was today so gotta focus on spinning that down. Sorry for the delay. 🤪

On Fri, Mar 29, 2024 at 5:58 PM Alessandro Jeanteur < @.***> wrote:

This fix looks amazing, thank you for getting that over the finish line! I don't know what the general policy is here for cutting new tags, but I wondered when this would be tagged and published to npm as a new version? there hasn't been a new version in a while so I'd like to know whether it's best for us to wait for 8.12 or to use this commit. Thanks again!

— Reply to this email directly, view it on GitHub https://github.com/brianc/node-postgres/pull/2836#issuecomment-2027806203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAMHIMJMIJCSCHOAMJ43KLY2XW25AVCNFSM6AAAAAAQ63TZT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHAYDMMRQGM . You are receiving this because you modified the open/close state.Message ID: @.***>