GODRIVER-2037 Don't clear the connection pool on client-side connect timeout errors.

[matthewdale]

Update topology.Server#ProcessHandshakeError to not clear the server connection pool on client-side timeout errors that occur while creating a connection in-line with an operation. This is intended as a patch to prevent clearing the connection pool on specific identified conditions that currently cause the driver to clear the server connection pool unnecessarily and is not intended to handle a comprehensive set of errors that could occur during handshake.
Note that the changes here are likely irrelevant once all connections are established in the background with the connectTimeoutMS deadline (GODRIVER-2038).

Possible cases:

1. server.Connection(context.Background())
   Get a connection to server using connectTimeoutMS. The server connection pool will be cleared on timeout during handshake. This case is used for connections created by the MinPoolSize maintenance goroutine and by operations that use context.Background() for the timeout.
2. server.Connection(context.WithTimeout(..., 1*time.Second))
   Get a connection to server using a timeout specified on an operation lower than connectTimeoutMS. The server connection pool will not be cleared on timeout during handshake. This case is used for connections created by operations that use context.WithTimeout() for timeouts shorter than the connectTimeoutMS.
3. server.Connection(context.WithTimeout(..., 10*time.Minute))
   Get a connection to server using a timeout specified on an operation higher than connectTimeoutMS. The server connection pool will be cleared on timeout during handshake. This case is used for connections created by operations that use context.WithTimeout() for timeouts longer than connectTimeoutMS.
4. server.Connection(context.WithCancel(...))
   Get a connection to server using a cancellation specified on an operation. The server connection pool will not be cleared on timeout during handshake. This case is used for connections created by operations that use context.WithCancel() for cancellation.

GODRIVER-2037

[kevinAlbs]

Looking great!

[kevinAlbs]

Fantastic work, LGTM!

[divjotarora]

Looks great! Just some minor comments about testing.

[divjotarora]

Looks great! The new operationTimeout parameter for tests is much clearer IMO. I responded to a previous thread about testing context.Canceled errors so I'll hold off on approving until that's resolved, but everything else pretty much LGTM.

[matthewdale]

@kevinAlbs and @divjotarora I've made 2 somewhat significant changes since the last time you reviewed this:

1. Pass the operation-scoped context to the ProcessHandshakeError function and check if the actual deadline has passed (if there is one) instead of just checking for ctx.Err(). This handles a race condition caused by some net functions that extract the deadline from a context and time out on that deadline, but the goroutine that closes ctx.Done() and causes ctx.Err() to return an error hasn't run yet.
2. Use individual instances of the new testPoolMonitor type to replace the package-global poolMonitor in the mongo/integration package tests. This fixes unexpected interaction between the pool monitor in different test cases that results in assertion failures and test timeouts (caused by infinitely blocking channel sends).

[divjotarora]

LGTM! Thanks for investigating this issue and all of the approaches so thoroughly!

[divjotarora]

nit: For readability, can you write a comment at the beginning of this test that explains what we're testing?

[matthewdale]

Will add comment

[kevinAlbs]

LGTM mod optional comments.

[kevinAlbs]

Oh very nice. Optional: should this be in a separate file, like connection_pool_helpers_test.go? Though isPoolCleared was used by other files, the name of this file suggests it is only for the tests specified in https://github.com/mongodb/specifications/blob/master/source/connections-survive-step-down/tests/README.rst

[matthewdale]

Yeah, I wanted to locate it "next to" the existing poolMonitor so that there aren't two implementations of the same thing in different places. I think the testPoolMonitor could actually be a good candidate for a "test utilities" package because numerous test packages need to record events (including the Server tests added in this PR). However, I'm not sure it's worth moving into a separate file until we want to move it into a test utilities package, which probably shouldn't be part of this PR.

I've added investigating moving testPoolMonitor to a shared test utilities package to the description of GODRIVER-2068..

[kevinAlbs]

Thank you for the helpful explanation! That is interesting, and sounds like it could be possible buggy behavior in the net package if it inconsistently sets context errors.

[kevinAlbs]

Optional: IIUC this could use the new testPoolMonitor.

[matthewdale]

Yes it could, although it would require moving testPoolMonitor into a test util package like internal/testutil (or even an external test utilities package if that makes sense). I'll leave a comment to indicate that change should be considered as part of GODRIVER-2068.