GODRIVER-1540 fix deadlock in connection #348

iwysiu · 2020-03-26T17:44:11Z

No description provided.

craiggwilson · 2020-03-26T19:14:22Z

x/mongo/driver/topology/pool_test.go

@@ -518,6 +520,34 @@ func TestPool(t *testing.T) {
 			}
 			close(cleanup)
 		})
+		t.Run("handshaker i/o fails", func(t *testing.T) {


Just a question here... does this deadlock without the change?

craiggwilson · 2020-03-26T19:16:13Z

x/mongo/driver/topology/connection.go

-		if c.nc != nil {
-			err = c.nc.Close()
+	if c.pool != nil {
+		err := c.pool.removeConnection(c)


So, the previous call to c.pool.closeConnection did a lot of other things too, such as closing the underlying connection (c.nc.Close())... do we need to be doing at least some of that as well?

The old call to c.pool.closeConnection would try to remove the connectionID first, and if that worked would close the underlying connection, which we're still doing.

Ah, I see it now. I was having some trouble reading the diff, but this conditional calls c.pool.removeConnection and the code below closes c.nc. The removeConnection call really shouldn't ever error because the only error is if the pool doesn't match this connection's pool, but would it make more sense logically to close nc first? That way, this function guarantees that the underlying resources are always closed, even if an error occurs.

@divjotarora done

ahcobos-rappi · 2020-03-27T00:16:25Z

+1

divjotarora

One minor comment, but otherwise LGTM.

divjotarora · 2020-03-27T14:28:44Z

x/mongo/driver/topology/connection.go

-		if c.nc != nil {
-			err = c.nc.Close()
+	if c.pool != nil {
+		err := c.pool.removeConnection(c)


Ah, I see it now. I was having some trouble reading the diff, but this conditional calls c.pool.removeConnection and the code below closes c.nc. The removeConnection call really shouldn't ever error because the only error is if the pool doesn't match this connection's pool, but would it make more sense logically to close nc first? That way, this function guarantees that the underlying resources are always closed, even if an error occurs.

* Update version to v1.3.1+prerelease * Change w value for unsatisfiable write concern in tests (mongodb#301) - Addresses test failures that started after by SERVER-45920 * GODRIVER-895 check if strLength is 0 in ReadCodeWithScope (mongodb#311) * GODRIVER-1487 fix adding UnknownTransactionCommitResult (mongodb#307) * GODRIVER-1488 turn off DecodeDeepZeroInline for mgocompat.Registry (mongodb#312) * Fix primitive.Regex Equal method (mongodb#318) * GODRIVER-1507 Correctly pass URI to topology (mongodb#320) * Update version to 1.3.1 * Update version to v1.3.2+prerelease * GODRIVER-1522 Ignore read preference for aggregations with output stages (mongodb#327) * GODRIVER-1520 Fix panics for lone scope (mongodb#331) * GODRIVER-1532 Remove check when decoding invalid UTF-8 strings (mongodb#333) * Fix typo in objectid.go (mongodb#335) * GODRIVER-1502 Fix variable shadowing in parsing (mongodb#332) * GODRIVER-1506 Fix error checking for invalid extjson timestamp values (mongodb#337) * GODRIVER-1504 Restrict top-level keys from being parsed as extjson (mongodb#340) * GODRIVER-1513 Disallow escaped single quotes in extjson keys (mongodb#342) * GODRIVER-1535 Fix session IDs batching in Disconnect * GODRIVER-1540 fix deadlock in connection (mongodb#348) * GODRIVER-1549 Select all servers when an empty tag set is given (mongodb#352) * GODRIVER-1431 Add explicit encryption examples (mongodb#350) * Update version to v1.3.2 * STITCH-4296: Export contextWithSession * STITCH-4296: Export sessionFromContext * STITCH-4680 - expose topology consistency * STITCH-4841: Add FullDocumentBeforeChange option to Mongo-go-driver * GODRIVER-672 Change session IDs to be stored as bson.Raw (mongodb#339) (cherry picked from commit d080bd0) * do not leak cancelConnectContext * REALMC-5872 renaming NewSessionContext to ContextWithSession * REALMC-5872 adding back size check in pool test * REALMC-5872 removing unused IsConsistent topology function and renaming changed vars Co-authored-by: Divjot Arora <[email protected]> Co-authored-by: iwysiu <[email protected]> Co-authored-by: mengskysama <[email protected]> Co-authored-by: Luca(Wei) Chen <[email protected]> Co-authored-by: Haley Owen <[email protected]> Co-authored-by: Eric Daniels <[email protected]> Co-authored-by: Tyler Kaye <[email protected]> Co-authored-by: Jonathan Reams <[email protected]> Co-authored-by: mike o'brien <[email protected]>

* Update version to v1.3.1+prerelease * Change w value for unsatisfiable write concern in tests (mongodb#301) - Addresses test failures that started after by SERVER-45920 * GODRIVER-895 check if strLength is 0 in ReadCodeWithScope (mongodb#311) * GODRIVER-1487 fix adding UnknownTransactionCommitResult (mongodb#307) * GODRIVER-1488 turn off DecodeDeepZeroInline for mgocompat.Registry (mongodb#312) * Fix primitive.Regex Equal method (mongodb#318) * GODRIVER-1507 Correctly pass URI to topology (mongodb#320) * Update version to 1.3.1 * Update version to v1.3.2+prerelease * GODRIVER-1522 Ignore read preference for aggregations with output stages (mongodb#327) * GODRIVER-1520 Fix panics for lone scope (mongodb#331) * GODRIVER-1532 Remove check when decoding invalid UTF-8 strings (mongodb#333) * Fix typo in objectid.go (mongodb#335) * GODRIVER-1502 Fix variable shadowing in parsing (mongodb#332) * GODRIVER-1506 Fix error checking for invalid extjson timestamp values (mongodb#337) * GODRIVER-1504 Restrict top-level keys from being parsed as extjson (mongodb#340) * GODRIVER-1513 Disallow escaped single quotes in extjson keys (mongodb#342) * GODRIVER-1535 Fix session IDs batching in Disconnect * GODRIVER-1540 fix deadlock in connection (mongodb#348) * GODRIVER-1549 Select all servers when an empty tag set is given (mongodb#352) * GODRIVER-1431 Add explicit encryption examples (mongodb#350) * Update version to v1.3.2 * STITCH-4296: Export contextWithSession * STITCH-4296: Export sessionFromContext * STITCH-4680 - expose topology consistency * STITCH-4841: Add FullDocumentBeforeChange option to Mongo-go-driver * GODRIVER-672 Change session IDs to be stored as bson.Raw (mongodb#339) (cherry picked from commit d080bd0) * do not leak cancelConnectContext * REALMC-5872 renaming NewSessionContext to ContextWithSession * REALMC-5872 adding back size check in pool test * REALMC-5872 removing unused IsConsistent topology function and renaming changed vars Co-authored-by: Divjot Arora <[email protected]> Co-authored-by: iwysiu <[email protected]> Co-authored-by: mengskysama <[email protected]> Co-authored-by: Luca(Wei) Chen <[email protected]> Co-authored-by: Haley Owen <[email protected]> Co-authored-by: Eric Daniels <[email protected]> Co-authored-by: Tyler Kaye <[email protected]> Co-authored-by: Jonathan Reams <[email protected]> Co-authored-by: mike o'brien <[email protected]> (cherry picked from commit c59bda0)

* REALMC-5872 merging topology changes to v1.4.1 driver (#1) * Update version to v1.3.1+prerelease * Change w value for unsatisfiable write concern in tests (mongodb#301) - Addresses test failures that started after by SERVER-45920 * GODRIVER-895 check if strLength is 0 in ReadCodeWithScope (mongodb#311) * GODRIVER-1487 fix adding UnknownTransactionCommitResult (mongodb#307) * GODRIVER-1488 turn off DecodeDeepZeroInline for mgocompat.Registry (mongodb#312) * Fix primitive.Regex Equal method (mongodb#318) * GODRIVER-1507 Correctly pass URI to topology (mongodb#320) * Update version to 1.3.1 * Update version to v1.3.2+prerelease * GODRIVER-1522 Ignore read preference for aggregations with output stages (mongodb#327) * GODRIVER-1520 Fix panics for lone scope (mongodb#331) * GODRIVER-1532 Remove check when decoding invalid UTF-8 strings (mongodb#333) * Fix typo in objectid.go (mongodb#335) * GODRIVER-1502 Fix variable shadowing in parsing (mongodb#332) * GODRIVER-1506 Fix error checking for invalid extjson timestamp values (mongodb#337) * GODRIVER-1504 Restrict top-level keys from being parsed as extjson (mongodb#340) * GODRIVER-1513 Disallow escaped single quotes in extjson keys (mongodb#342) * GODRIVER-1535 Fix session IDs batching in Disconnect * GODRIVER-1540 fix deadlock in connection (mongodb#348) * GODRIVER-1549 Select all servers when an empty tag set is given (mongodb#352) * GODRIVER-1431 Add explicit encryption examples (mongodb#350) * Update version to v1.3.2 * STITCH-4296: Export contextWithSession * STITCH-4296: Export sessionFromContext * STITCH-4680 - expose topology consistency * STITCH-4841: Add FullDocumentBeforeChange option to Mongo-go-driver * GODRIVER-672 Change session IDs to be stored as bson.Raw (mongodb#339) (cherry picked from commit d080bd0) * do not leak cancelConnectContext * REALMC-5872 renaming NewSessionContext to ContextWithSession * REALMC-5872 adding back size check in pool test * REALMC-5872 removing unused IsConsistent topology function and renaming changed vars Co-authored-by: Divjot Arora <[email protected]> Co-authored-by: iwysiu <[email protected]> Co-authored-by: mengskysama <[email protected]> Co-authored-by: Luca(Wei) Chen <[email protected]> Co-authored-by: Haley Owen <[email protected]> Co-authored-by: Eric Daniels <[email protected]> Co-authored-by: Tyler Kaye <[email protected]> Co-authored-by: Jonathan Reams <[email protected]> Co-authored-by: mike o'brien <[email protected]> (cherry picked from commit c59bda0) * REALMC-6991: add topology check Co-authored-by: Kush Patel <[email protected]>

GODRIVER-1540 fix deadlock in connection

043cedb

iwysiu requested review from jyemin and divjotarora March 26, 2020 17:44

jyemin requested a review from craiggwilson March 26, 2020 18:37

craiggwilson reviewed Mar 26, 2020

View reviewed changes

iwysiu requested a review from craiggwilson March 27, 2020 14:24

divjotarora approved these changes Mar 27, 2020

View reviewed changes

jyemin removed their request for review March 27, 2020 17:23

divjot comment

6443ba5

craiggwilson approved these changes Mar 27, 2020

View reviewed changes

iwysiu merged commit a2fd877 into mongodb:master Mar 27, 2020

iwysiu deleted the GODRIVER-1540 branch March 27, 2020 20:34

iwysiu pushed a commit that referenced this pull request Mar 27, 2020

GODRIVER-1540 fix deadlock in connection (#348)

df8f93d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

GODRIVER-1540 fix deadlock in connection #348

GODRIVER-1540 fix deadlock in connection #348

Uh oh!

iwysiu commented Mar 26, 2020

Uh oh!

craiggwilson Mar 26, 2020

Uh oh!

iwysiu Mar 27, 2020

Uh oh!

craiggwilson Mar 26, 2020

Uh oh!

iwysiu Mar 27, 2020

Uh oh!

divjotarora Mar 27, 2020

Uh oh!

iwysiu Mar 27, 2020

Uh oh!

ahcobos-rappi commented Mar 27, 2020

Uh oh!

divjotarora left a comment

Uh oh!

divjotarora Mar 27, 2020

Uh oh!

Uh oh!

GODRIVER-1540 fix deadlock in connection #348

GODRIVER-1540 fix deadlock in connection #348

Uh oh!

Conversation

iwysiu commented Mar 26, 2020

Uh oh!

craiggwilson Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

iwysiu Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

craiggwilson Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

iwysiu Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

divjotarora Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

iwysiu Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

ahcobos-rappi commented Mar 27, 2020

Uh oh!

divjotarora left a comment

Choose a reason for hiding this comment

Uh oh!

divjotarora Mar 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!