Update err for single conn pool when context.Done() return first #1981

xin-tsla · 2023-06-20T07:33:52Z

Hi All,
The issue this PR tries to solve is that we observed an NPE in OnConnect method

Line 119 in f7e1c98

return db.opt.OnConnect(ctx, newConn(ctx, db.withPool(p)))

when we call conn.Exec() twice inside the OnConnect method. and the second call (conn.Exec()) may get an nil conn object and fails to call conn.Exec()[panic].

The data flow diagram depicts how the conn is nil in some case:

If ctx gets cancelled and returns before query finishes (Yellow line in the diagram), the conn in SingleConnPool will call Remove method to set the conn to nil:

pg/internal/pool/pool_single.go

Line 38 in f7e1c98

p.cn = nil

and as well to set an error with reason, which is nil since it is due to ctx gets cancelled without an error:

pg/internal/pool/pool_single.go

Line 39 in f7e1c98

p.stickyErr = reason

When ctx is cancelled:

pg/base.go

Line 153 in f7e1c98

case <-ctx.Done():

and send struct{}{} to fnDone:

pg/base.go

Line 159 in f7e1c98

fnDone <- struct{}{}

elliotcourant

Overall this looks good to me, I have a things I'd want to just tweak/get clarification on. But I'm happy to merge this once thats ready!

elliotcourant · 2023-06-20T15:51:08Z

internal/pool/pool_single_test.go

+	var p *pool.SingleConnPool
+
+	BeforeEach(func() {
+		p = pool.NewSingleConnPool(nil, &pool.Conn{})
+	})


Would this need to be a before each and a variable in this scope? Or could the pool just get moved into the test itself since there is only one?

Good point! It is more clean, addressed: 9653a28

elliotcourant · 2023-06-20T15:51:23Z

internal/pool/pool_single_test.go

+	})
+
+	It("remove a conn due to context is cancelled", func() {
+		ctx, cl := context.WithCancel(context.TODO())


I'd rename cl to cancel to make it clear here.

Yes, renamed: ec32de7

elliotcourant · 2023-06-20T15:51:45Z

internal/pool/pool_single_test.go

+		Expect(cn).To(BeNil())
+		Expect(err).ToNot(BeNil())
+	})
+


nit pick, no need for this extra newline

Addressed along with a previous commit 9653a28

internal/pool/pool_single.go

xin-tsla · 2023-06-23T18:41:38Z

Hi @elliotcourant , wondering when you have time, could you take a second look?

elliotcourant · 2023-06-23T21:21:05Z

Thank you very much for the fix @xin-tsla !

xin-tsla · 2023-06-23T21:22:42Z

@elliotcourant , happy to work on the fix! And much appreciate your review and time! :)

xin-tsla · 2023-06-26T17:36:19Z

@elliotcourant , when you have time, another question, could you create a new release version?

elliotcourant · 2023-06-26T19:52:49Z

I can tag it for release this week, gonna try to loop in some other changes with that release as well. Mostly dependency bumps.

xin-tsla · 2023-06-26T20:04:40Z

@elliotcourant , got it. Thank you so much for tagging the release! :)

xin-tsla · 2023-07-03T16:39:26Z

@elliotcourant , by chance when you have time, could you tag a new release version?

elliotcourant · 2023-07-08T18:46:40Z

@xin-tsla Sorry for the delay! It is now tagged!

xin-tsla · 2023-07-08T19:26:11Z

@elliotcourant , no worries! Thanks for tagging it! Much appreciate your time!

* Warn on unknown tags * Change Logger interface * Add RealWorld example app * using go.opentelemetry.io/otel @v0.11.0 see https://github.com/open-telemetry/opentelemetry-go/releases/tag/v0.11.0 * ran go mod tidy * Add msgpack option * Set span name. Updates go-pg#1699 * Add nopk field option * Increase read buffer size to 1mb and reuse it via sync.Pool * feat(otel): Use first word as span name without operation. * Cleanup code * Update sponsors * Update * Add docs badge * Fix badge * Extract pgext to separate Go module * Add pgext to readme * Update changelog * Split channel and ping timeout * Change chan Notification * Fix test * . * Add ability to scan into map[string]interface{} * Add json_use_number to the list of known options * Test on Go 1.15 * Fix map update and insert * Fix test on older Postgres * Add support for scanning into map slice * Cleanup todo * Update changelog * Update sponsors to changelog * Restore column alloc * Add types.Numeric * Add Discord * Update issue templates * Update issue template * Disable blank template * Add stackoverflow linke * Cleanup * Better naming * Cleanup * Add self-validating rel tag option * Update changelog * Rework rel * Cleanup example * Update example * Update example names * Fix structs sharing in has-many * Add basic example * Update changelog * Fix discord link * orm: warn on tag name mistake * Add renovate.json * chore(deps): update golang.org/x/exp commit hash to ae8ad44 * chore(deps): update module onsi/ginkgo to v1.14.1 * Fix m2m table name containing a schema * Update module onsi/gomega to v1.10.2 * Fix a typo in changelog * Change hooks to be LIFO * Make sure to call after hook on error * Fix fk handling for has-one rel * Use primary key as fallback in m2m * Fix m2m using column without a prefix * dont-swallow-before-query-errors * Update golang.org/x/exp commit hash to 20d5ce0 * Update example names * Fix example name * Don't use retry backoff in listener * Add go vet * Update golang.org/x/exp commit hash to 18d7dbd * Add discord badge * Parse field tag only once when check for relation * Implement soft delete for scanner * Tweak scanning into a map * Fix build * Reset map slice len before scanning data * Add HexEncoder * Properly pass data len in scanner * fix many2many for multi-tenant applications (table names with a param) * dependency update and workaround for opentelemetry api change * Cleanup * Fix default + use_zero insert * Improve known tag check * Add treemux * Update module vmihailenco/tagparser to v0.1.2 * Cleanup * More resources * Update module go.opentelemetry.io/otel to v0.13.0 * Update readme * Update module onsi/ginkgo to v1.14.2 * Update module onsi/gomega to v1.10.3 * orm: try SQL name as fk. Fixes go-pg#1757 * Update dependencies * Add alias to the list of known options * Add extra modules * Remove pgext * chore(deps): update module go-pg/pg/v10 to v10.5.0 * Improve scanning null values * Don't reuse column name * Feat: Remove byte slice in ColumnAlloc * Remove unused column * Add an optional EmptyLine in log * Move changelog to docs * Tweak otel span and attrs name * Fix build * Try funding * Otel v0.14 * orm: improve WherePK for custom types * Fix batch update and delete * Fix WherePK for slices * Properly trim extra comma * chore(deps): update module go.opentelemetry.io/otel to v0.15.0 * Add go-pg-monitor to README ecosystem section * Bump dependencies * Undone timeout and cancel in Rollback/Commit context * Fix logger prefix * (go-pg#1830) Fixed notice during startup error. This adds handling of Notice messages during startup and will log the messages for the client to see so they may take action. * Remove renovate * Bumped otel up to v0.17.0 * Bumped otel up to v0.17.0 Bumped up testify t0 v1.7.0 * Update dependencies * Add utm * Add banner * Otel v0.18 * Fix otel db.system name According to the open [telemetry specification](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes): > db.system MUST be one of the following or, if none of the listed values apply, a custom value: Down the list the name for postgres reads as "postgresql". Add the remaining "ql" characters and conform the expectations of the spec. * Add a link to discussions * Fix discussions link * Use unformatted query when over limit * Update dependencies * Remove confusing statement * orm: improve template to omits columns when there are plenty * Add DBI * Move changelog back to the repo * pgotel: add hard query limit * Make extra modules to follow go-pg versioning * Update changelog * Add NewJSONProvider * extra/pgotel: add readme and example * Fix title * OpenTelemetry v0.20.0 * Add link to Bun * Mark more errors as fatal * Remove OpenTelemetry instrumentation from the core (but keep pgotel as is) * Update OpenTelemetry * Always remove canceled conn from the pool * Add big.Int link * int[] type to array conversion * Update example_model_test.go * Add link to Bun * Add possibility to specify custom `join_fk` for "has-one" relations. Will close go-pg#1812. * Replace travis * Add support for ScramSha256Plus * gofmt and go mod tidy * Release v10.10.4 (release.sh) * Version v10.10.4 (tag.sh) * Fix SASL authentication * Release v10.10.5 (release.sh) * Replace release script * Update dependencies * Update changelog * Release v10.10.6 (release.sh) * chore: tweak readme * fix: Add `-race` flag to GitHub actions test workflow. Added `-race` to the `test` target in the Makefile, this way tests that are run in GitHub Actions will help reveal any potential race conditions. * chore: update links * chore: update link * chore: update readme * chore: add monetr * Fix a bug for sending cancel request to terminate a long running query (go-pg#1952) * Add for loop for checking if the query is finished or is cancelled * Rename a var from RetryStatementTimeout to RetryCancelledQuery and update a comment * Send a cancel request when there is a bad connection error * Rename a var back to RetryStatementTimeout * Revert a change in withConn * Revert changes for comments * Add an unit test for testing sending a cancel request when a connection is timed out * Change the return values of isBadConn to return error code if it has * Send a cancel request when it is a bad conn expect 25P02 case * chore(ci): Bumping CI versions for Go. (go-pg#1955) * Add shortcut WhereInOr (go-pg#1947) * Add shortcut WhereInOr * Add tests for WhereInOr Co-authored-by: p.vorontsov <p.vorontsov@norsi-trans.ru> * chore: cleanup * chore: add release scripts (go-pg#1957) * fix(notify): Fixed race condition in listener. (go-pg#1963) This resolves a race condition in listener where `ln.closed` was being read without a mutex at the same time it could be written. Just move the mutex unlock for `Unlisten` down, **but not defer it** so that the mutex still holds the lock properly. But does not cause a deadlock with release conn. * Release v10.10.7 (release.sh) (go-pg#1964) * chore: update * chore: update * Bump mellium.im/sasl from 0.2.1 to 0.3.1 (go-pg#1969) * Bump mellium.im/sasl from 0.2.1 to 0.3.1 Bumps mellium.im/sasl from 0.2.1 to 0.3.1. --- updated-dependencies: - dependency-name: mellium.im/sasl dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * chore: Removing Go 1.16 support. Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Elliot Courant <elliot@treasuryprime.com> * chore: Make release scripts work on mac OS * Release v10.11.0 (release.sh) (go-pg#1970) * Bump golang.org/x/sys from 0.0.0-20210923061019-b8560ed6a9b7 to 0.1.0 (go-pg#1974) * Bump golang.org/x/crypto from 0.0.0-20210921155107-089bfa567519 to 0.1.0 (go-pg#1975) * Update err for single conn pool when context.Done() return first (go-pg#1981) * Update err from ctx if reason is nil * Add a unit test * Move pool object to the test itself * Rename a var from cl to cancel * Add comment for explaining why ctx.Err() is used * Release v10.11.1 (release.sh) (go-pg#1982) --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Vladimir Mihailenco <vladimir.webdev@gmail.com> Co-authored-by: burner-account <github1@depke.net> Co-authored-by: j2gg0s <wangyanjie@rcrai.com> Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: ferhat elmas <elmas.ferhat@gmail.com> Co-authored-by: Jeremy Mickelson <jmickelson@tesla.com> Co-authored-by: Shoshin Nikita <shoshin_nikita@fastmail.com> Co-authored-by: Martin Ottenwaelter <martin.ottenwaelter@gmail.com> Co-authored-by: burner-account <gitlab1@depke.net> Co-authored-by: j2gg0s <j2gg0s@gmail.com> Co-authored-by: frederikhors <41120635+frederikhors@users.noreply.github.com> Co-authored-by: Igor Zibarev <zibarev.i@gmail.com> Co-authored-by: Anatoly Rugalev <anatoly.rugalev@gmail.com> Co-authored-by: Elliot Courant <me@elliotcourant.dev> Co-authored-by: Marvin Hansen <marvin.hansen@gmail.com> Co-authored-by: Matthew Nibecker <hello@mattnibecker.com> Co-authored-by: alexander <alexander@saporo.io> Co-authored-by: 047801 <61029048+iam047801@users.noreply.github.com> Co-authored-by: Vladimir Stolyarov <xakep6666@gmail.com> Co-authored-by: xin-tsla <107882527+xin-tsla@users.noreply.github.com> Co-authored-by: Elliot Courant <elliot@treasuryprime.com> Co-authored-by: swad2007 <3888@list.ru> Co-authored-by: p.vorontsov <p.vorontsov@norsi-trans.ru> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

xin-tsla added 2 commits June 18, 2023 00:29

Update err from ctx if reason is nil

2d24b5c

Add a unit test

62d7cd2

elliotcourant requested changes Jun 20, 2023

View reviewed changes

xin-tsla added 2 commits June 20, 2023 11:36

Move pool object to the test itself

9653a28

Rename a var from cl to cancel

ec32de7

xin-tsla force-pushed the xin/v10-update-err-for-signle-conn-pool branch from 171dd1b to ec32de7 Compare June 20, 2023 18:40

Add comment for explaining why ctx.Err() is used

2755f6d

xin-tsla requested a review from elliotcourant June 20, 2023 21:51

elliotcourant approved these changes Jun 23, 2023

View reviewed changes

elliotcourant merged commit 4e20be7 into go-pg:v10 Jun 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update err for single conn pool when context.Done() return first #1981

Update err for single conn pool when context.Done() return first #1981

xin-tsla commented Jun 20, 2023

elliotcourant left a comment

elliotcourant Jun 20, 2023

xin-tsla Jun 20, 2023 •

edited

Loading

elliotcourant Jun 20, 2023

xin-tsla Jun 20, 2023

elliotcourant Jun 20, 2023

xin-tsla Jun 20, 2023 •

edited

Loading

xin-tsla commented Jun 23, 2023

elliotcourant commented Jun 23, 2023

xin-tsla commented Jun 23, 2023

xin-tsla commented Jun 26, 2023

elliotcourant commented Jun 26, 2023

xin-tsla commented Jun 26, 2023

xin-tsla commented Jul 3, 2023

elliotcourant commented Jul 8, 2023

xin-tsla commented Jul 8, 2023

Update err for single conn pool when context.Done() return first #1981

Update err for single conn pool when context.Done() return first #1981

Conversation

xin-tsla commented Jun 20, 2023

elliotcourant left a comment

Choose a reason for hiding this comment

elliotcourant Jun 20, 2023

Choose a reason for hiding this comment

xin-tsla Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

elliotcourant Jun 20, 2023

Choose a reason for hiding this comment

xin-tsla Jun 20, 2023

Choose a reason for hiding this comment

elliotcourant Jun 20, 2023

Choose a reason for hiding this comment

xin-tsla Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

xin-tsla commented Jun 23, 2023

elliotcourant commented Jun 23, 2023

xin-tsla commented Jun 23, 2023

xin-tsla commented Jun 26, 2023

elliotcourant commented Jun 26, 2023

xin-tsla commented Jun 26, 2023

xin-tsla commented Jul 3, 2023

elliotcourant commented Jul 8, 2023

xin-tsla commented Jul 8, 2023

xin-tsla Jun 20, 2023 •

edited

Loading

xin-tsla Jun 20, 2023 •

edited

Loading