Skip to content

Conversation

@STRML
Copy link
Contributor

@STRML STRML commented Dec 3, 2019

Removes the process.nextTick() wrapping the release callback after calling pg.Pool#connect().

This was questioned in this comment and never removed. We found test flakiness that was alleviated after removing this unnecessary tick.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Commit messages are following our guidelines

@STRML STRML changed the title Fix/conn release fix(pool): synchronously release pool connection after query results Fix/conn release Dec 3, 2019
Copy link
Member

@dhmlau dhmlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@STRML, thanks for the PR. Your changes look reasonable to me.
I'm not sure if we can add some tests for that. Would like to get other maintainers to review it as well. Thanks.

@dhmlau
Copy link
Member

dhmlau commented Dec 4, 2019

Just realized the errors after approving the PR. @STRML, could you please fix the commit linter error? Thanks.

**************************************************
**
**  Linting commit logs
**
**  2 problems found:
**    d32e3e6 - fix(pool): synchronously release pool connection af: First line should be 50 characters or less (saw 68)
**    d32e3e6 - fix(pool): synchronously release pool connection af: line 4 longer than 72 characters.
**
**************************************************

@dhmlau dhmlau added the community-contribution Patches contributed by community label Dec 4, 2019
@dhmlau dhmlau self-requested a review December 4, 2019 16:33
Copy link
Contributor

@elv1s elv1s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good after fixing the commit message

@STRML
Copy link
Contributor Author

STRML commented Dec 6, 2019

Fixed commit message.

@bajtos
Copy link
Member

bajtos commented Dec 9, 2019

**************************************************
**
**  Linting commit logs
**
**  1 problems found:
**    6ebfcb9 - fix(pool): synchronously release pool connection: line 4 longer than 72 characters.
**
**************************************************

To speed up the process, I am going to fix the commit message myself.

Rapidly-fired queries can exhaust the pool capability and lead to odd
results. In test, we've witnessed "Connection Terminated" flakiness
with many fast test suites. This, combined with setting
`lazyConnect: true` and `idleTimeoutMillis: 0`, alleviates the issue.
@bajtos
Copy link
Member

bajtos commented Dec 9, 2019

14:07:46   1) autoupdate
14:07:46        foreign key constraint
14:07:46          should create, update, and delete foreign keys:
14:07:46      error: duplicate key value violates unique constraint "pg_namespace_nspname_index" (while running initial autoupdate)
14:07:46       at Connection.parseE (node_modules/pg/lib/connection.js:606:11)
14:07:46       at Connection.parseMessage (node_modules/pg/lib/connection.js:403:19)
14:07:46       at Socket.<anonymous> (node_modules/pg/lib/connection.js:123:22)
14:07:46       at addChunk (_stream_readable.js:263:12)
14:07:46       at readableAddChunk (_stream_readable.js:250:11)
14:07:46       at Socket.Readable.push (_stream_readable.js:208:10)
14:07:46       at TCP.onread (net.js:601:20)

The build failure looks unrelated to me, let's re-run the tests again.

IIRC, we are observing this error from time to time, it would be great if somebody could investigate the problem and contribute a fix. /cc @strongloop/loopback-connector-postgresql-maintainers

@bajtos
Copy link
Member

bajtos commented Dec 9, 2019

@slnode test please

@bajtos bajtos merged commit 98bccd2 into loopbackio:master Dec 9, 2019
@bajtos
Copy link
Member

bajtos commented Dec 9, 2019

Landed, thank you @STRML for the contribution ❤️

@bajtos
Copy link
Member

bajtos commented Dec 9, 2019

Published as loopback-connector-postgresql@3.8.1 🚀

@STRML STRML deleted the fix/conn-release branch December 10, 2019 05:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Patches contributed by community

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants