Page MenuHomePhabricator

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

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

Details

Summary

Ref T13216. See PHI916. Harbormaster builds may be long-running, particularly if they effectively wrap ssh ... ./run-huge-build.sh. 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

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

Event Timeline

epriestley created this revision.Tue, Nov 20, 7:03 PM
Owners added a subscriber: Restricted Owners Package.Tue, Nov 20, 7:03 PM
epriestley requested review of this revision.Tue, Nov 20, 7:05 PM
epriestley marked 3 inline comments as done.Tue, Nov 20, 7:09 PM
epriestley added inline comments.
src/applications/search/index/PhabricatorIndexEngine.php
141–147

Unrelated qsprintf() fix.

src/applications/search/ngrams/PhabricatorSearchNgrams.php
105–107

Unrelated qsprintf() fix.

src/infrastructure/storage/lisk/LiskDAO.php
1655–1658

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.Tue, Nov 20, 10:09 PM
amckinley added inline comments.
src/infrastructure/storage/lisk/LiskDAO.php
1657

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.Tue, Nov 20, 10:09 PM
epriestley marked an inline comment as done.Tue, Nov 20, 10:21 PM
epriestley added inline comments.
src/infrastructure/storage/lisk/LiskDAO.php
1657

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.