Separate read/write network timeouts #161

domodwyer · 2018-05-09T16:59:01Z

Fixes #160.

Defines two new DialInfo fields: ReadTimeout, and WriteTimeout, defaulting to DialInfo.Timeout for backwards compatibility.

Removes passing multiple copies of the timeout value throughout the codebase, instead keeping a reference to a copy of the DialInfo which should make future refactoring easier (all the information is available everywhere, rather than ad-hock passing in function arguments).

As a nice bonus, because the same DialInfo instance is referenced by all sockets rather than each socket referencing its own copy of the timeout value in the hot path, the increased cache locality has reduced operation latency by ~15% resulting in ~1700/s increase in read throughput.

efe0945164a7e582241f37ae8983c075f8f2e870-timeout-insert-latency.txt
efe0945164a7e582241f37ae8983c075f8f2e870-timeout-insert-throughput.txt
efe0945164a7e582241f37ae8983c075f8f2e870-timeout-select-zipfian-latency.txt
efe0945164a7e582241f37ae8983c075f8f2e870-timeout-select-zipfian-throughput.txt
efe0945164a7e582241f37ae8983c075f8f2e870-timeout-update-zipfian-latency.txt
efe0945164a7e582241f37ae8983c075f8f2e870-timeout-update-zipfian-throughput.txt

Splits DialInfo.Timeout (defaults to 60s when using mgo.Dial()) into ReadTimeout and WriteTimeout to address #160. Read/write timeout defaults to DialInfo.Timeout to preserve existing behaviour.

Only used by tests, replaced by the pool-aware acquire socket functions: * AcquireSocketWithPoolTimeout * AcquireSocketWithBlocking

* `mongoCluster.syncServer()` no longer uses hard-coded 5 seconds * `mongoCluster.isMaster()` no longer uses hard-coded 10 seconds

When unmarshalling serverTags, it is now an empty slice, instead of a nil slice. `len(thing) == 0` works all the time, regardless.

szank · 2018-05-09T17:18:13Z

cluster.go

@@ -386,7 +374,7 @@ func (cluster *mongoCluster) syncServersLoop() {
 			break
 		}
 		cluster.references++ // Keep alive while syncing.
-		direct := cluster.direct
+		direct := cluster.dialInfo.Direct


cluster.dialInfo is a pointer, so it being inside the cluster lock does not make sense. ( I might be missing something ), but I think it would be more intuitive to move this assignment out of the critical section.

I've kept the locking semantics from the existing code, it was confusing enough tracing the timeouts around the codebase without taking into account the locking choices - the locking needs a review in general.

szank · 2018-05-10T10:00:14Z

session.go

+	// Only limits the network read - does not include unmarshalling /
+	// processing of the response. Defaults to DialInfo.Timeout. If 0, no
+	// timeout is set.
+	ReadTimeout time.Duration


The wording can be a bit misleading IMHO. What is happening is:

if ReadTimeout > 0, use it
2 else if DialInfo.Timeout > 0 use it

else no timeout is set, wait indefinitely ?

I would word it like this:

// Only limits the network read - does not include unmarshalling / // processing of the response. Defaults to DialInfo.Timeout. If , ReadTimeout is 0 and Timeout is 0 no // timeout is set.

I think your comment is missing words, but the flow you describe is what the existing comment says? I tested it on the DB guy.

You are right. Ignore it please.

szank · 2018-05-10T10:04:36Z

socket.go

@@ -223,7 +225,7 @@ func (socket *mongoSocket) ServerInfo() *mongoServerInfo {
 // InitialAcquire obtains the first reference to the socket, either
 // right after the connection is made or once a recycled socket is
 // being put back in use.
-func (socket *mongoSocket) InitialAcquire(serverInfo *mongoServerInfo, timeout time.Duration) error {
+func (socket *mongoSocket) InitialAcquire(serverInfo *mongoServerInfo, dialInfo *DialInfo) error {


The *mongoSocket already contains dialInfo at the time of this call as far as I can tell (maybe there is another place where it doesn't (?))

It can be called from a sync task that can have a different timeout configured, so it respects the value it was provided rather than the socket timeout.

szank

Need to add a comment to change the review status ( comment -> Request changes)

eminano · 2018-05-10T10:54:11Z

session.go

+	return i.ReadTimeout
+}
+
+// writeTimeout returns the configured write timeout, or DefaultWriteTimeout if


DefaultWriteTimeout is not used, the same goes for DefaultReadTimeout

Thanks! I removed all this logic to maintain backwards compatibility and forgot the consts.

eminano · 2018-05-10T11:02:45Z

session.go

@@ -595,17 +710,9 @@ func DialWithInfo(info *DialInfo) (*Session, error) {
 		}
 		session.creds = []Credential{*session.dialCred}
 	}
-	if info.PoolLimit > 0 {
-		session.poolLimit = info.PoolLimit
-	}

 	cluster.minPoolSize = info.MinPoolSize


Why do you need cluster to have minPoolSize and maxIdleTimeMS when you have it in the DialInfo (you're using their values anyway)?

Refactoring oversight, they're unused so I'll remove them completely.

tadukurow · 2018-05-10T09:42:54Z

server.go

 	for {
 		server.Lock()
 		abended = server.abended
 		if server.closed {
 			server.Unlock()
 			return nil, abended, errServerClosed
 		}
-		if poolLimit > 0 {
+		if info.poolLimit() > 0 {


calling of timeout methods and members seems to be mixed and it's not very clear why.
Are you trying to avoid overhead of calling checking logic in the methods?

Yeah want to keep the default value handling DRY during refactoring but on second thought it could go into the DialWithInfo and avoid the runtime branching.

tadukurow · 2018-05-10T11:04:38Z

session.go

+	// connections in the connection pool.
+	//
+	// To override this value set DialInfo.PoolLimit.
+	DefaultConnectionPoolSize = 4096


Not perfect either but DefaultConnectionPoolLimit is easier to relate back to PoolLimit.

tadukurow · 2018-05-10T11:10:38Z

session.go

 	PoolTimeout time.Duration

+	// ReadTimeout defines the maximum duration to wait for a response from


It includes reading the response right?

ReadTimeout defines the maximum duration to wait for a response to be read from

tadukurow · 2018-05-10T11:12:34Z

session.go

+// poolLimit returns the configured connection pool size, or
+// DefaultConnectionPoolSize.
+func (i *DialInfo) poolLimit() int {
+	if i == nil || i.PoolLimit == 0 {


defending against nil pointer here, but not in the methods above.

tadukurow · 2018-05-10T11:18:35Z

session.go

+	return info
+}
+
+// readTimeout returns the configured read timeout, or DefaultReadTimeout if


It doesn't return DefaultReadTimeout when unset

tadukurow · 2018-05-10T11:18:53Z

session.go

+
+// writeTimeout returns the configured write timeout, or DefaultWriteTimeout if
+// unset.
+func (i *DialInfo) writeTimeout() time.Duration {


it doesn't return DefaultWriteTimeout when unset

tadukurow · 2018-05-10T11:23:28Z

session.go

+	// DefaultReadTimeout is set to 60 seconds for backwards compatibility.
+	//
+	// See DialInfo.ReadTimeout
+	DefaultReadTimeout = time.Second * 60


DefaultReadTimeout and DefaultWriteTimeout seem to be unused.

tadukurow · 2018-05-10T11:50:59Z

cluster.go

-func (cluster *mongoCluster) AcquireSocketWithPoolTimeout(
-	mode Mode, slaveOk bool, syncTimeout time.Duration, socketTimeout time.Duration, serverTags []bson.D, poolLimit int, poolTimeout time.Duration,
-) (s *mongoSocket, err error) {
+func (cluster *mongoCluster) AcquireSocketWithPoolTimeout(mode Mode, slaveOk bool, syncTimeout time.Duration, serverTags []bson.D, info *DialInfo) (s *mongoSocket, err error) {


Can syncTimeout be moved to DialInfo as well?

It takes more of my life than I am willing to give.

Changes `DialWithInfo` to handle setting default values by setting the relevant `DialInfo` field, rather than calling the respective methods in the hot path for: * `PoolLimit` * `ReadTimeout` * `WriteTimeout`

stale

@tbruyelle

…lease/r2018.06.15 * 'development' of ssh://github.com/globalsign/mgo: readme: credit @tbruyelle (#190) Add support for ssl dial string (#184) revert: MGO-156 Avoid iter.Next deadlock on dead sockets (#182) (#188) readme: credit everyone (#187) Contributing:findAndModify support writeConcern (#185) Allow passing slice pointer as an interface pointer to Iter.All (#181) MGO-156 Avoid iter.Next deadlock on dead sockets (#182) add NewMongoTimestamp() and MongoTimestamp.Time(),Counter() (#171) Expand documentation for *Iter.Next (#163) Add Collation support for calling Count() on a Query (#166) add URI options: "w", "j", "wtimeoutMS" (#162) Separate read/write network timeouts (#161) Respect nil slices, maps in bson encoder (#147) fix(dbtest): Use os.Kill on windows instead of Interrupt 🐛 inline pointer_to_struce mode: update comments. return error on pointer not to struct allow ptr in inline structs

@changwoo-nam

* allow ptr in inline structs * inline pointer_to_struce mode: update comments. return error on pointer not to struct * fix(dbtest): Use os.Kill on windows instead of Interrupt 🐛 I've added a use for os.Kill, instead of os.Interrupt signal, when using Windows. I'm current developing my project on Windows, and using DBServer.Stop() was resulting in: "timeout waiting for mongod process to die". After investigating, I've discovered that os.Interrupt isn't implemented on Windows, and it seems golang has Frozen this issue due to age (2013). They instruct to use os.Kill instead. Using this, the DBServer on my project works with no problem. * Respect nil slices, maps in bson encoder (#147) * socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * added support for marshalling/unmarshalling maps with non-string keys * refactor method receiver * added support for json-compatible support for slices and maps Marshal() func: nil slice or map converts to nil, not empty (initialized with len=0) * fix IsNil on slices and maps format * added godoc * fix sasl empty payload * fix scram-sha-1 auth * revert fix sasl empty payload * Separate read/write network timeouts (#161) * socket: separate read/write network timeouts Splits DialInfo.Timeout (defaults to 60s when using mgo.Dial()) into ReadTimeout and WriteTimeout to address #160. Read/write timeout defaults to DialInfo.Timeout to preserve existing behaviour. * cluster: remove AcquireSocket Only used by tests, replaced by the pool-aware acquire socket functions: * AcquireSocketWithPoolTimeout * AcquireSocketWithBlocking * cluster: use configured timeouts for cluster operations * `mongoCluster.syncServer()` no longer uses hard-coded 5 seconds * `mongoCluster.isMaster()` no longer uses hard-coded 10 seconds * tests: use DialInfo for internal timeouts * server: fix fantastic serverTags nil slice bug When unmarshalling serverTags, it is now an empty slice, instead of a nil slice. `len(thing) == 0` works all the time, regardless. * cluster: remove unused duplicate pool config * session: avoid calculating default values in hot path Changes `DialWithInfo` to handle setting default values by setting the relevant `DialInfo` field, rather than calling the respective methods in the hot path for: * `PoolLimit` * `ReadTimeout` * `WriteTimeout` * session: remove unused consts * session: update docs * add URI options: "w", "j", "wtimeoutMS" (#162) * add URI options: "w", "j", "wtimeoutMS" * change "w" to "j" * Add Collation support for calling Count() on a Query (#166) * Expand documentation for *Iter.Next (#163) The documentation now explains the difference between calling Err and Close after Next returns false. The example code has been expanded to include checking for timeout. * add NewMongoTimestamp() and MongoTimestamp.Time(),Counter() (#171) code is inspired by go-mgo#202 * MGO-156 Avoid iter.Next deadlock on dead sockets (#182) * Allow passing slice pointer as an interface pointer to Iter.All (#181) * socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * Allow passing slice pointer as an interface pointer to Iter.All * Reverted to original error message, added test case for interface{} ptr * Contributing:findAndModify support writeConcern (#185) * socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * findAndModify support writeConcern * fix * readme: credit everyone (#187) * @cedric-cordenier * @DaytonG * @ddspog * @gedge * @jefferickson * @larrycinnabar * @Mei-Zhao * @roobre * revert: MGO-156 Avoid iter.Next deadlock on dead sockets (#182) (#188) This reverts commit 7253b2b. * Add support for ssl dial string (#184) * Add support for ssl dial string * Ensure we dont override user settings * update examples * update ssl value parsing * PingSsl test * skip test requiring system certificates * readme: credit @tbruyelle (#190)

@changwoo-nam

* socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * Release/r2018.06.15 (#191) * allow ptr in inline structs * inline pointer_to_struce mode: update comments. return error on pointer not to struct * fix(dbtest): Use os.Kill on windows instead of Interrupt 🐛 I've added a use for os.Kill, instead of os.Interrupt signal, when using Windows. I'm current developing my project on Windows, and using DBServer.Stop() was resulting in: "timeout waiting for mongod process to die". After investigating, I've discovered that os.Interrupt isn't implemented on Windows, and it seems golang has Frozen this issue due to age (2013). They instruct to use os.Kill instead. Using this, the DBServer on my project works with no problem. * Respect nil slices, maps in bson encoder (#147) * socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * added support for marshalling/unmarshalling maps with non-string keys * refactor method receiver * added support for json-compatible support for slices and maps Marshal() func: nil slice or map converts to nil, not empty (initialized with len=0) * fix IsNil on slices and maps format * added godoc * fix sasl empty payload * fix scram-sha-1 auth * revert fix sasl empty payload * Separate read/write network timeouts (#161) * socket: separate read/write network timeouts Splits DialInfo.Timeout (defaults to 60s when using mgo.Dial()) into ReadTimeout and WriteTimeout to address #160. Read/write timeout defaults to DialInfo.Timeout to preserve existing behaviour. * cluster: remove AcquireSocket Only used by tests, replaced by the pool-aware acquire socket functions: * AcquireSocketWithPoolTimeout * AcquireSocketWithBlocking * cluster: use configured timeouts for cluster operations * `mongoCluster.syncServer()` no longer uses hard-coded 5 seconds * `mongoCluster.isMaster()` no longer uses hard-coded 10 seconds * tests: use DialInfo for internal timeouts * server: fix fantastic serverTags nil slice bug When unmarshalling serverTags, it is now an empty slice, instead of a nil slice. `len(thing) == 0` works all the time, regardless. * cluster: remove unused duplicate pool config * session: avoid calculating default values in hot path Changes `DialWithInfo` to handle setting default values by setting the relevant `DialInfo` field, rather than calling the respective methods in the hot path for: * `PoolLimit` * `ReadTimeout` * `WriteTimeout` * session: remove unused consts * session: update docs * add URI options: "w", "j", "wtimeoutMS" (#162) * add URI options: "w", "j", "wtimeoutMS" * change "w" to "j" * Add Collation support for calling Count() on a Query (#166) * Expand documentation for *Iter.Next (#163) The documentation now explains the difference between calling Err and Close after Next returns false. The example code has been expanded to include checking for timeout. * add NewMongoTimestamp() and MongoTimestamp.Time(),Counter() (#171) code is inspired by go-mgo#202 * MGO-156 Avoid iter.Next deadlock on dead sockets (#182) * Allow passing slice pointer as an interface pointer to Iter.All (#181) * socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * Allow passing slice pointer as an interface pointer to Iter.All * Reverted to original error message, added test case for interface{} ptr * Contributing:findAndModify support writeConcern (#185) * socket: only send client metadata once per socket (#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (#111) * Brings in a patch on having flusher not suppress errors. (#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes #101 and fixes #103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (#110) * Hotfix #120 (#136) * cluster: fix deadlock in cluster synchronisation (#120) For a impressively thorough breakdown of the problem, see: #120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * findAndModify support writeConcern * fix * readme: credit everyone (#187) * @cedric-cordenier * @DaytonG * @ddspog * @gedge * @jefferickson * @larrycinnabar * @Mei-Zhao * @roobre * revert: MGO-156 Avoid iter.Next deadlock on dead sockets (#182) (#188) This reverts commit 7253b2b. * Add support for ssl dial string (#184) * Add support for ssl dial string * Ensure we dont override user settings * update examples * update ssl value parsing * PingSsl test * skip test requiring system certificates * readme: credit @tbruyelle (#190) * strip space of flag

@changwoo-nam

* allow ptr in inline structs * inline pointer_to_struce mode: update comments. return error on pointer not to struct * fix(dbtest): Use os.Kill on windows instead of Interrupt 🐛 I've added a use for os.Kill, instead of os.Interrupt signal, when using Windows. I'm current developing my project on Windows, and using DBServer.Stop() was resulting in: "timeout waiting for mongod process to die". After investigating, I've discovered that os.Interrupt isn't implemented on Windows, and it seems golang has Frozen this issue due to age (2013). They instruct to use os.Kill instead. Using this, the DBServer on my project works with no problem. * Respect nil slices, maps in bson encoder (globalsign#147) * socket: only send client metadata once per socket (globalsign#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes globalsign#101 and fixes globalsign#103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (globalsign#111) * Brings in a patch on having flusher not suppress errors. (globalsign#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (globalsign#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes globalsign#101 and fixes globalsign#103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (globalsign#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (globalsign#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (globalsign#110) * Hotfix globalsign#120 (globalsign#136) * cluster: fix deadlock in cluster synchronisation (globalsign#120) For a impressively thorough breakdown of the problem, see: globalsign#120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * added support for marshalling/unmarshalling maps with non-string keys * refactor method receiver * added support for json-compatible support for slices and maps Marshal() func: nil slice or map converts to nil, not empty (initialized with len=0) * fix IsNil on slices and maps format * added godoc * fix sasl empty payload * fix scram-sha-1 auth * revert fix sasl empty payload * Separate read/write network timeouts (globalsign#161) * socket: separate read/write network timeouts Splits DialInfo.Timeout (defaults to 60s when using mgo.Dial()) into ReadTimeout and WriteTimeout to address globalsign#160. Read/write timeout defaults to DialInfo.Timeout to preserve existing behaviour. * cluster: remove AcquireSocket Only used by tests, replaced by the pool-aware acquire socket functions: * AcquireSocketWithPoolTimeout * AcquireSocketWithBlocking * cluster: use configured timeouts for cluster operations * `mongoCluster.syncServer()` no longer uses hard-coded 5 seconds * `mongoCluster.isMaster()` no longer uses hard-coded 10 seconds * tests: use DialInfo for internal timeouts * server: fix fantastic serverTags nil slice bug When unmarshalling serverTags, it is now an empty slice, instead of a nil slice. `len(thing) == 0` works all the time, regardless. * cluster: remove unused duplicate pool config * session: avoid calculating default values in hot path Changes `DialWithInfo` to handle setting default values by setting the relevant `DialInfo` field, rather than calling the respective methods in the hot path for: * `PoolLimit` * `ReadTimeout` * `WriteTimeout` * session: remove unused consts * session: update docs * add URI options: "w", "j", "wtimeoutMS" (globalsign#162) * add URI options: "w", "j", "wtimeoutMS" * change "w" to "j" * Add Collation support for calling Count() on a Query (globalsign#166) * Expand documentation for *Iter.Next (globalsign#163) The documentation now explains the difference between calling Err and Close after Next returns false. The example code has been expanded to include checking for timeout. * add NewMongoTimestamp() and MongoTimestamp.Time(),Counter() (globalsign#171) code is inspired by go-mgo#202 * MGO-156 Avoid iter.Next deadlock on dead sockets (globalsign#182) * Allow passing slice pointer as an interface pointer to Iter.All (globalsign#181) * socket: only send client metadata once per socket (globalsign#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes globalsign#101 and fixes globalsign#103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (globalsign#111) * Brings in a patch on having flusher not suppress errors. (globalsign#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (globalsign#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes globalsign#101 and fixes globalsign#103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (globalsign#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (globalsign#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (globalsign#110) * Hotfix globalsign#120 (globalsign#136) * cluster: fix deadlock in cluster synchronisation (globalsign#120) For a impressively thorough breakdown of the problem, see: globalsign#120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * Allow passing slice pointer as an interface pointer to Iter.All * Reverted to original error message, added test case for interface{} ptr * Contributing:findAndModify support writeConcern (globalsign#185) * socket: only send client metadata once per socket (globalsign#105) Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes globalsign#101 and fixes globalsign#103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Merge Development (globalsign#111) * Brings in a patch on having flusher not suppress errors. (globalsign#81) go-mgo#360 * Fallback to JSON tags when BSON tag isn't present (globalsign#91) * Fallback to JSON tags when BSON tag isn't present Cleanup. * Add test to demonstrate tagging fallback. - Test coverage for tagging test. * socket: only send client metadata once per socket Periodic cluster synchronisation calls isMaster() which currently resends the "client" metadata every call - the spec specifies: isMaster commands issued after the initial connection handshake MUST NOT contain handshake arguments https://github.com/mongodb/specifications/blob/master/source/mongodb-handshake/handshake.rst#connection-handshake This hotfix prevents subsequent isMaster calls from sending the client metadata again - fixes globalsign#101 and fixes globalsign#103. Thanks to @changwoo-nam @qhenkart @canthefason @jyoon17 for spotting the initial issue, opening tickets, and having the problem debugged with a PoC fix before I even woke up. * Cluster abended test 254 (globalsign#100) * Add a test that mongo Server gets their abended reset as necessary. See https://github.com/go-mgo/mgo/issues/254 and https://github.com/go-mgo/mgo/pull/255/files * Include the patch from Issue 255. This brings in a test which fails without the patch, and passes with the patch. Still to be tested, manual tcpkill of a socket. * changeStream support (globalsign#97) Add $changeStream support * readme: credit @peterdeka and @steve-gray (globalsign#110) * Hotfix globalsign#120 (globalsign#136) * cluster: fix deadlock in cluster synchronisation (globalsign#120) For a impressively thorough breakdown of the problem, see: globalsign#120 (comment) Huge thanks to @dvic and @KJTsanaktsidis for the report and fix. * readme: credit @dvic and @KJTsanaktsidis * findAndModify support writeConcern * fix * readme: credit everyone (globalsign#187) * @cedric-cordenier * @DaytonG * @ddspog * @gedge * @jefferickson * @larrycinnabar * @Mei-Zhao * @roobre * revert: MGO-156 Avoid iter.Next deadlock on dead sockets (globalsign#182) (globalsign#188) This reverts commit 7253b2b. * Add support for ssl dial string (globalsign#184) * Add support for ssl dial string * Ensure we dont override user settings * update examples * update ssl value parsing * PingSsl test * skip test requiring system certificates * readme: credit @tbruyelle (globalsign#190)

domodwyer added 5 commits May 9, 2018 17:17

socket: separate read/write network timeouts

775e6d7

Splits DialInfo.Timeout (defaults to 60s when using mgo.Dial()) into ReadTimeout and WriteTimeout to address #160. Read/write timeout defaults to DialInfo.Timeout to preserve existing behaviour.

cluster: remove AcquireSocket

95f2333

Only used by tests, replaced by the pool-aware acquire socket functions: * AcquireSocketWithPoolTimeout * AcquireSocketWithBlocking

cluster: use configured timeouts for cluster operations

545befd

* `mongoCluster.syncServer()` no longer uses hard-coded 5 seconds * `mongoCluster.isMaster()` no longer uses hard-coded 10 seconds

tests: use DialInfo for internal timeouts

3b17aab

server: fix fantastic serverTags nil slice bug

c0b9052

When unmarshalling serverTags, it is now an empty slice, instead of a nil slice. `len(thing) == 0` works all the time, regardless.

domodwyer added bug enhancement needs stg test labels May 9, 2018

domodwyer self-assigned this May 9, 2018

domodwyer requested review from szank, brknstrngz, tadukurow, weiishann, csucu and eminano May 9, 2018 16:59

szank reviewed May 10, 2018

View reviewed changes

szank suggested changes May 10, 2018

View reviewed changes

szank previously approved these changes May 10, 2018

View reviewed changes

eminano previously requested changes May 10, 2018

View reviewed changes

tadukurow previously requested changes May 10, 2018

View reviewed changes

domodwyer added 3 commits May 11, 2018 13:23

cluster: remove unused duplicate pool config

fbe8acd

session: avoid calculating default values in hot path

2573a57

Changes `DialWithInfo` to handle setting default values by setting the relevant `DialInfo` field, rather than calling the respective methods in the hot path for: * `PoolLimit` * `ReadTimeout` * `WriteTimeout`

session: remove unused consts

716ee13

domodwyer dismissed szank’s stale review via 716ee13 May 11, 2018 12:58

session: update docs

22be2e3

szank approved these changes May 11, 2018

View reviewed changes

eminano approved these changes May 11, 2018

View reviewed changes

tadukurow approved these changes May 11, 2018

View reviewed changes

csucu approved these changes May 11, 2018

View reviewed changes

domodwyer merged commit 72d0ac2 into development May 11, 2018

domodwyer deleted the feature/granular-timeout-control branch May 11, 2018 13:55

domodwyer mentioned this pull request May 23, 2018

Race condition on minPoolSize and maxIdleTimeMS #173

Closed

seriousben mentioned this pull request May 30, 2018

Data race on Dial #178

Closed

domodwyer mentioned this pull request Jun 15, 2018

Release/r2018.06.15 #191

Merged

QieKai mentioned this pull request Aug 8, 2018

Release/r2018.06.15 (#191) nzgogo/mgo#2

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate read/write network timeouts #161

Separate read/write network timeouts #161

domodwyer commented May 9, 2018 •

edited

Loading

szank May 9, 2018 •

edited

Loading

domodwyer May 10, 2018

szank May 10, 2018

domodwyer May 10, 2018

szank May 10, 2018

szank May 10, 2018

domodwyer May 10, 2018

szank left a comment

eminano May 10, 2018

domodwyer May 11, 2018

eminano May 10, 2018

domodwyer May 11, 2018

tadukurow May 10, 2018

domodwyer May 11, 2018

tadukurow May 10, 2018

tadukurow May 10, 2018

tadukurow May 10, 2018

tadukurow May 10, 2018

tadukurow May 10, 2018

tadukurow May 10, 2018

tadukurow May 10, 2018

domodwyer May 11, 2018

		PoolTimeout time.Duration

		// ReadTimeout defines the maximum duration to wait for a response from

Separate read/write network timeouts #161

Separate read/write network timeouts #161

Conversation

domodwyer commented May 9, 2018 • edited Loading

szank May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domodwyer commented May 9, 2018 •

edited

Loading

szank May 9, 2018 •

edited

Loading