A bad assumption made me think that was what is happening.
I can't reproduce the hanging connections locally with launchpad trunk. Looking back at the mailing list thread it looks like it would be difficult to reproduce unless under load with a proper sized database:
""" This works by having the DB URI as a config variable that returns the
appropriate value according to the presence of the read-only.txt file,
together with a publication hook that forces the storm stores to
reconnect whenever there's a mode switch.
However, storm (through ZStorm) shares storm.database.Database instances
across threads (without saying so in the docs) and our custom version of
that class (LaunchpadDatabase) may change its DB URI (when there's a
config change) after instantiation[2]. This, in itself, is a problem,
but since we don't do runtime config changes except when running the
test suite (which is single-threaded), it's never affected us.
My changes for the read-only switch, though, introduced a way of
changing config values at runtime, thus exposing the problem.
This means there's a race condition when switching to read-only mode,
when a thread kicks in after another thread has noticed the config
change and reconnected its stores, thus causing the shared instance of
LaunchpadDatabase to change its DB URI. When other threads kick in,
they'll think they're connected to the correct DB (remember we use the
DB URI stored in LaunchpadDatabase for that) and won't tell their stores
to reconnect, leaving open connections to DBs that should have no
connections.
"""
The good news is that my fix sorts this too I think, as we no longer use the DB URI to detect if we have switched too or from read-only mode (and as an additional benefit, we no longer need to poke at Storm internals per the existing comment in the code).
A bad assumption made me think that was what is happening.
I can't reproduce the hanging connections locally with launchpad trunk. Looking back at the mailing list thread it looks like it would be difficult to reproduce unless under load with a proper sized database:
""" This works by having the DB URI as a config variable that returns the
appropriate value according to the presence of the read-only.txt file,
together with a publication hook that forces the storm stores to
reconnect whenever there's a mode switch.
However, storm (through ZStorm) shares storm.database. Database instances
across threads (without saying so in the docs) and our custom version of
that class (LaunchpadDatabase) may change its DB URI (when there's a
config change) after instantiation[2]. This, in itself, is a problem,
but since we don't do runtime config changes except when running the
test suite (which is single-threaded), it's never affected us.
My changes for the read-only switch, though, introduced a way of
changing config values at runtime, thus exposing the problem.
This means there's a race condition when switching to read-only mode, tabase to change its DB URI. When other threads kick in,
when a thread kicks in after another thread has noticed the config
change and reconnected its stores, thus causing the shared instance of
LaunchpadDa
they'll think they're connected to the correct DB (remember we use the
DB URI stored in LaunchpadDatabase for that) and won't tell their stores
to reconnect, leaving open connections to DBs that should have no
connections.
"""
The good news is that my fix sorts this too I think, as we no longer use the DB URI to detect if we have switched too or from read-only mode (and as an additional benefit, we no longer need to poke at Storm internals per the existing comment in the code).