Page MenuHomePhabricator

When waiting for long-running Harbormaster futures to resolve, close idle database connections

Authored by epriestley on Nov 20 2018, 7:03 PM.



Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap ssh ... ./ If we spend more than a few seconds waiting for futures to resolve, close idle database connections.

The general goal here is to reduce the held connection load for installs with a very large number of test runners.

Test Plan

Added debugging code to phlog() closures, saw connections closed while running builds.

Diff Detail

rP Phabricator
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Nov 20 2018, 7:03 PM
Owners added a subscriber: Restricted Owners Package.Nov 20 2018, 7:03 PM
epriestley requested review of this revision.Nov 20 2018, 7:05 PM
epriestley marked 3 inline comments as done.Nov 20 2018, 7:09 PM
epriestley added inline comments.

Unrelated qsprintf() fix.


Unrelated qsprintf() fix.


This is just adding some safety. Today, closeInactiveConnections() could possibly close a connection with a transaction or a lock, but we only call it after workers finish, when it's always "reasonably safe" to close any open transactions or release any locks (it's still "wrong" in some sense, but the worker itself is in the wrong if it can exit with held transactions or held locks).

So: before, closeInactiveConnections() could be unsafe, but was only called in a safe way. Now, closeInactiveConnections() is safe to call from anywhere.

(Possibly closeIdleConnections() should just be implemented as closeInactiveConnections(0) -- that is, close inactive connections with at least 0 seconds of inactivity -- but the ideas seemed slightly different.)

amckinley accepted this revision.Nov 20 2018, 10:09 PM
amckinley added inline comments.

Maybe throw or log here, since we should in theory never reach this, and is definitely indicative of a bug if we do?

This revision is now accepted and ready to land.Nov 20 2018, 10:09 PM
epriestley marked an inline comment as done.Nov 20 2018, 10:21 PM
epriestley added inline comments.

It's no longer bad to reach this. If we reached it today that would be a little surprising/unexpected, but we (or third parties) are now free to call this method from wherever they feel like.

(Oh, and connections getting cleaned up with open transactions [or held locks?] already throw/log separately, although I'm not sure this behavior is actually useful since it has caught ~0 transaction bugs and causes ~a million "symptom" exceptions where something else is the problem, throws, causes a connection to close with an open transaction, and then the log is muddied by the transaction message. So I might possibly remove or reduce the severity of the transaction complaining in the future.)

I think it's usually hard to (a) forget to saveTransaction() and (b) not notice that your code has no effect when you test it. Easier if there's some complicated nest of conditions and exception handling, but at least relatively more difficult than "any of your other code throws".

This revision was automatically updated to reflect the committed changes.