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

alxndrsn · 2022-10-06T17:53:10Z

Fix for #1674, alternative to #2832.

brianc · 2022-11-21T17:56:36Z

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 · 2022-11-23T13:31:44Z

Thanks @brianc - all feedback very appreciated 🙂

alxndrsn · 2023-02-21T09:47:17Z

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

alxndrsn · 2023-04-28T12:38:12Z

Thanks @rafaell-lycan!

nathanjcochran · 2023-05-25T18:00:31Z

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 · 2023-05-26T08:59:06Z

I believe this PR would fix my problem.

@nathanjcochran I see you have a neat repro at #1674 (comment); any chance you can test this patch against that?

nathanjcochran · 2023-05-26T15:32:24Z

I see you have a neat repro at #1674 (comment); any chance you can test this patch against that?

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

alxndrsn · 2023-05-26T15:37:48Z

@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 · 2023-05-26T16:17:32Z

@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 · 2023-05-29T13:34:54Z

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

alxndrsn · 2023-05-29T13:55:06Z

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

alxndrsn · 2023-05-29T14:14:09Z

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 · 2023-05-30T15:02:19Z

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

@alxndrsn Go for it! Thank you!!

alxndrsn · 2023-06-01T05:55:11Z

Thanks @nathanjcochran - added 👍

alxndrsn · 2023-07-05T08:57:31Z

@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 · 2023-07-05T17:10:14Z

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 · 2023-09-20T06:59:53Z

@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 · 2023-09-27T15:16:00Z

@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 · 2023-10-11T12:58:02Z

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

@nathanjcochran

Includes test case from @nathanjcochran

This reverts commit 5f5d42a.

alxndrsn · 2023-10-20T10:54:02Z

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)

…-twice

alxndrsn · 2024-03-14T08:05:19Z

I've merged the latest master into this branch and the tests pass ✔️

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

This should prevent runaway builds. I noticed this issue when working on a fork of brianc#2836. Example builds with/without these timeouts: 1. 6 hours: https://github.com/alxndrsn/node-postgres/actions/runs/8277192701 2. 10 minutes: https://github.com/alxndrsn/node-postgres/actions/runs/8277388503 These timeouts are 4-5x what a current healthy build takes.

This should prevent runaway builds. I noticed this issue when working on a fork of #2836. Example builds with/without these timeouts: 1. 6 hours: https://github.com/alxndrsn/node-postgres/actions/runs/8277192701 2. 10 minutes: https://github.com/alxndrsn/node-postgres/actions/runs/8277388503 These timeouts are 4-5x what a current healthy build takes.

brianc · 2024-03-15T17:25:56Z

Thank you so much! Your help is amazing!

alessbelli · 2024-03-29T22:58:33Z

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 · 2024-03-30T02:34:09Z

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 <#2836 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAMHIMJMIJCSCHOAMJ43KLY2XW25AVCNFSM6AAAAAAQ63TZT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHAYDMMRQGM> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

brianc · 2024-03-30T20:52:11Z

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 > <#2836 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAMHIMJMIJCSCHOAMJ43KLY2XW25AVCNFSM6AAAAAAQ63TZT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRXHAYDMMRQGM> > . > You are receiving this because you modified the open/close state.Message > ID: ***@***.***> >

alxndrsn marked this pull request as ready for review October 6, 2022 17:56

alxndrsn mentioned this pull request Jan 10, 2023

Error: Cannot read property 'name' of null #2705

Open

rafaell-lycan approved these changes Mar 13, 2023

View reviewed changes

nathanjcochran mentioned this pull request May 25, 2023

Errored QueryStream causes Client to throw on message receive #1674

Closed

alxndrsn force-pushed the dont-close-cursor-twice branch from 63e1515 to eae0b69 Compare May 29, 2023 14:13

This was referenced Jul 5, 2023

pg-pool: don't allow connection reuse after streaming #2832

Closed

stream-use-after-error: failing test case #2829

Closed

alxndrsn added 3 commits October 20, 2023 10:09

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

57d0de4

Includes test case from @nathanjcochran

Resolve lint violations

c7e766a

revert fix to check tests fail without it

5f5d42a

alxndrsn force-pushed the dont-close-cursor-twice branch from cd51583 to 5f5d42a Compare October 20, 2023 10:19

Re-introdce fix

0867aad

This reverts commit 5f5d42a.

Merge remote-tracking branch 'upstream/master' into dont-close-cursor…

59b1e07

…-twice

alxndrsn mentioned this pull request Mar 14, 2024

ci: add build timeouts #3172

Merged

brianc merged commit 91de4b9 into brianc:master Mar 15, 2024
6 checks passed

lognaturel mentioned this pull request Mar 27, 2024

Get off of slonik fork getodk/central-backend#1119

Open

alxndrsn deleted the dont-close-cursor-twice branch March 17, 2024 06:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alxndrsn commented Oct 6, 2022 •

edited

Loading

brianc commented Nov 21, 2022

alxndrsn commented Nov 23, 2022

alxndrsn commented Feb 21, 2023

alxndrsn commented Apr 28, 2023

nathanjcochran commented May 25, 2023

alxndrsn commented May 26, 2023

nathanjcochran commented May 26, 2023

alxndrsn commented May 26, 2023

nathanjcochran commented May 26, 2023 •

edited

Loading

alxndrsn commented May 29, 2023

alxndrsn commented May 29, 2023

alxndrsn commented May 29, 2023

nathanjcochran commented May 30, 2023

alxndrsn commented Jun 1, 2023

alxndrsn commented Jul 5, 2023 •

edited

Loading

charmander commented Jul 5, 2023

alxndrsn commented Sep 20, 2023

lognaturel commented Sep 27, 2023 •

edited

Loading

alxndrsn commented Oct 11, 2023

alxndrsn commented Oct 20, 2023

alxndrsn commented Mar 14, 2024 •

edited

Loading

brianc commented Mar 15, 2024

alessbelli commented Mar 29, 2024

brianc commented Mar 30, 2024 via email

brianc commented Mar 30, 2024 via email

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

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

Conversation

alxndrsn commented Oct 6, 2022 • edited Loading

brianc commented Nov 21, 2022

alxndrsn commented Nov 23, 2022

alxndrsn commented Feb 21, 2023

alxndrsn commented Apr 28, 2023

nathanjcochran commented May 25, 2023

alxndrsn commented May 26, 2023

nathanjcochran commented May 26, 2023

alxndrsn commented May 26, 2023

nathanjcochran commented May 26, 2023 • edited Loading

alxndrsn commented May 29, 2023

alxndrsn commented May 29, 2023

alxndrsn commented May 29, 2023

nathanjcochran commented May 30, 2023

alxndrsn commented Jun 1, 2023

alxndrsn commented Jul 5, 2023 • edited Loading

charmander commented Jul 5, 2023

alxndrsn commented Sep 20, 2023

lognaturel commented Sep 27, 2023 • edited Loading

alxndrsn commented Oct 11, 2023

alxndrsn commented Oct 20, 2023

alxndrsn commented Mar 14, 2024 • edited Loading

brianc commented Mar 15, 2024

alessbelli commented Mar 29, 2024

brianc commented Mar 30, 2024 via email

brianc commented Mar 30, 2024 via email

alxndrsn commented Oct 6, 2022 •

edited

Loading

nathanjcochran commented May 26, 2023 •

edited

Loading

alxndrsn commented Jul 5, 2023 •

edited

Loading

lognaturel commented Sep 27, 2023 •

edited

Loading

alxndrsn commented Mar 14, 2024 •

edited

Loading