enable shrink the socket pool size #87

gnawux · 2018-01-26T10:15:41Z

we found the mgo will allocate the pool size during burst traffic but won't
close the sockets any more until restart the client or server.

And the mongo document defines two related query options

By implementing these two options, it could shrink the pool to minPoolSize after
the sockets introduced by burst traffic timeout.

The idea comes from https://github.com/JodeZer/mgo , he investigated
this issue and provide the initial commits.

I found there are still some issue in sockets maintenance, and had a PR against
his repo JodeZer#1 .

This commit include JodeZer's commits and my fix, and I simplified the data structure.
What's in this commit could be described as this figure:

+------------------------+
|        Session         | <-------+ Add options here
+------------------------+

+------------------------+
|        Cluster         | <-------+ Add options here
+------------------------+

+------------------------+
|        Server          | <-------+*Add options here
|                        |          *add timestamp when recycle a socket  +---+
|          +-----------+ |    +---+ *periodically check the unused sockets    |
|          | shrinker  <------+          and reclaim the timeout sockets. +---+
|          +-----------+ |                                                    |
|                        |                                                    |
+------------------------+                                                    |
                                                                              |
+------------------------+                                                    |
|        Socket          | <-------+ Add a field for last used times+---------+
+------------------------+

we found the mgo will allocate the pool size during burst traffic but won't close the sockets any more until restart the client or server. And the mongo document defines two related query options - [minPoolSize](https://docs.mongodb.com/manual/reference/connection-string/#urioption.minPoolSize) - [maxIdleTimeMS](https://docs.mongodb.com/manual/reference/connection-string/#urioption.maxIdleTimeMS) By implementing these two options, it could shrink the pool to minPoolSize after the sockets introduced by burst traffic timeout. The idea comes from https://github.com/JodeZer/mgo , he investigated this issue and provide the initial commits. I found there are still some issue in sockets maintenance, and had a PR against his repo JodeZer#1 . This commit include JodeZer's commits and my fix, and I simplified the data structure. What's in this commit could be described as this figure: +------------------------+ | Session | <-------+ Add options here +------------------------+ +------------------------+ | Cluster | <-------+ Add options here +------------------------+ +------------------------+ | Server | <-------+*Add options here | | *add timestamp when recycle a socket +---+ | +-----------+ | +---+ *periodically check the unused sockets | | | shrinker <------+ and reclaim the timeout sockets. +---+ | +-----------+ | | | | | +------------------------+ | | +------------------------+ | | Socket | <-------+ Add a field for last used times+---------+ +------------------------+ Signed-off-by: Wang Xu <gnawux@gmail.com>

gnawux · 2018-01-26T10:50:56Z

In the testing matrix, Go 1.9/Mongo 3.4 failed. And I can't find how it related to this change. Could anyone help figuring out what happened?

domodwyer · 2018-01-28T20:47:25Z

Hi @gnawux

Great PR and I love a good diagram, thanks! I'm going to take a good look at this as it could impact performance, though I suppose only under decreasing load so might not be an issue.

As for the tests, they're just flakey. I restarted the build and it succeeded fine.

Thank you for taking the time to open a PR! It's really appreciated.

Dom

domodwyer · 2018-01-28T21:02:08Z

Is it possible to include a couple of unit and/or integration tests to cover the "shrinking" code path?

gnawux · 2018-01-29T07:57:25Z

Sure, I'd like to, but it may take some time :)

By the way, do you have any suggested docs or examples for the test cases of mgo that I could ref?

domodwyer · 2018-01-30T16:46:55Z

Hi @gnawux

That's would be great, thanks! The code looks fine, the tests are just for future maintainability though not such a problem with this change I suppose.

Either way, a basic example is TestPing() - your test case will want to:

Dial to mongo with shrinking enabled
Copy() a few sessions to create new connections
Close() all copied sessions
Wait/force a shrink
Check Stats.SocketsAlive to ensure the pool has shrunk to minPoolSize

I can help if you need! Thanks again for taking the time to help 👍

Dom

szank · 2018-02-04T23:30:51Z

cluster.go

@@ -419,7 +421,7 @@ func (cluster *mongoCluster) server(addr string, tcpaddr *net.TCPAddr) *mongoSer
 	if server != nil {
 		return server
 	}
-	return newServer(addr, tcpaddr, cluster.sync, cluster.dial)
+	return newServer(addr, tcpaddr, cluster.sync, cluster.dial, cluster.minPoolSize, cluster.maxIdleTimeMS)


It's a bit late, and I might be wrong here, but aren't you reading the cluster fields (minPoolSize, maxIdleTimeMs) outside of the read lock?

I assume they stay constant during the run of the application, but stil I would appreciate a comment here stating tha this is safe.

will do, thanks @szank

szank · 2018-02-04T23:35:06Z

session.go

@@ -366,6 +377,16 @@ func ParseURL(url string) (*DialInfo, error) {
 				doc = append(doc, bson.DocElem{Name: strings.TrimSpace(kvp[0]), Value: strings.TrimSpace(kvp[1])})
 			}
 			readPreferenceTagSets = append(readPreferenceTagSets, doc)
+		case "minPoolSize":
+			minPoolSize, err = strconv.Atoi(opt.value)
+			if err != nil {


Could you please return an error if the value is <0?
This is unlikely to happen, but it is still an invalid input and it would be better to notify the user instead of silently ignoring the values.

will update

szank · 2018-02-04T23:37:58Z

session.go

@@ -552,6 +583,15 @@ func DialWithInfo(info *DialInfo) (*Session, error) {
 	if info.PoolLimit > 0 {
 		session.poolLimit = info.PoolLimit
 	}
+
+	if info.MinPoolSize > 0 {


I think this checks should be reversed to return an error if MinPoolSize or MaxIdleTimeMS are <0

szank · 2018-02-04T23:42:40Z

server.go

+		}
+		now := time.Now()
+		end := 0
+		reclaimMap := map[*mongoSocket]bool{}


It is a minor nitpick, but map[*mongoSocket]struct{}{} is more idiomatic

szank · 2018-02-04T23:49:10Z

server.go

+			copy(next, server.unusedSockets[end:])
+			server.unusedSockets = next
+			remainSockets := []*mongoSocket{}
+			for _, s := range server.liveSockets {


It is late, and I might be missing something here, but it looks like you populate the reclaimMap with the unused sockets. Then you check if the liveSockets contain elements from the unusedSockets. Is it possible to liveSockets and unusedSockets to contain pointers to the same structs ?

If I understand correctly, the liveSockets includes both in-use sockets and unusedSockets, and when we reclaim the timeout elements in unusedSockets, they should be removed from liveSockets as well. If you don't remove them, there would lead to a "leakage", i.e. when you allocate new socket, there are full of live sockets but no unused, and no sockets could be allocated then. I observed the case during my local test.

Right, thanks for the clarification :).

gnawux force-pushed the release_idle_for_upstream branch from 7a3c22c to 677c2df Compare January 26, 2018 10:23

gnawux force-pushed the release_idle_for_upstream branch from 677c2df to a7f58c0 Compare January 26, 2018 10:27

domodwyer added the enhancement label Jan 28, 2018

domodwyer added the needs tests label Jan 30, 2018

domodwyer requested review from szank, brknstrngz, tadukurow, domodwyer, weiishann, csucu and eminano January 30, 2018 16:52

szank reviewed Feb 4, 2018

View reviewed changes

domodwyer closed this Feb 12, 2018

gnawux mentioned this pull request Feb 27, 2018

enable shrink the socket pool size #116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable shrink the socket pool size #87

enable shrink the socket pool size #87

gnawux commented Jan 26, 2018

gnawux commented Jan 26, 2018

domodwyer commented Jan 28, 2018 •

edited

Loading

domodwyer commented Jan 28, 2018

gnawux commented Jan 29, 2018

domodwyer commented Jan 30, 2018

szank Feb 4, 2018

gnawux Feb 5, 2018

szank Feb 4, 2018

gnawux Feb 5, 2018

szank Feb 4, 2018

szank Feb 4, 2018

szank Feb 4, 2018

gnawux Feb 5, 2018

szank Feb 5, 2018

enable shrink the socket pool size #87

enable shrink the socket pool size #87

Conversation

gnawux commented Jan 26, 2018

gnawux commented Jan 26, 2018

domodwyer commented Jan 28, 2018 • edited Loading

domodwyer commented Jan 28, 2018

gnawux commented Jan 29, 2018

domodwyer commented Jan 30, 2018

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 Jan 28, 2018 •

edited

Loading