Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #3411: Threaded access to Database #3422

Merged
merged 1 commit into from Jun 18, 2018

Conversation

@Kissaki
Copy link
Member

commented Jun 17, 2018

Qt5 documents that QSqlDatabase must not be called from varying threads.
An instance must be limited to its thread.

In Mumble the Database class handles database access.
Up to now it contained static methods and always used the global default
database instance.
We instantiate the default database connection into the Global context.
It is mostly used from the main event loop, but also from a thread in
ServerHandler.
This is broken as per specification, and Qt 5.11 seems to finally enforce
that.

To resolve this issue, we promote Database to an instantiable class,
use the created Global context class instance from the event loop,
and a distinct Database instance (and connection) from the ServerHandler
instance (and thread).


This replaces PR #3419.

@davidebeatrici Please not the elaborate and complete commit message.

Compared to the other PR, this PR suggests to use two explicit Database instances [and connections]. They are named, which is useful if we ever were to debug issues or errors. Identifying db connections by an arbitrary number (thread address) is not really feasible/immediately useful.

Being explicit is in most cases better than being generic. We now know who uses the database, and in which [thread] context.

This change brings us closer to the desired state of no global state. Previously we had unspecific unnamed global state (default database as per Qt). Now we use an explicit global state/instance with our Global class. If we ever manage to drop Global, this diff is at least a move in that direction.

Please note that this does not compile locally for me, but due to an unrelated non-change (somehow dirty compile workspace?). I just want to bring this out here now. CI can prove/disprove if it builds, or I'll check in the coming days.

WDYT?

@davidebeatrici

This comment has been minimized.

Copy link
Member

commented Jun 17, 2018

Everything seems good to me.

The no-pch builds are failing because <QSqlDatabase> has to be included.

Fix #3411: Threaded access to Database
Qt5 documents that QSqlDatabase must not be called from varying threads.
An instance must be limited to its thread.

In Mumble the Database class handles database access.
Up to now it contained static methods and always used the global default
database instance.
We instantiate the default database connection into the Global context.
It is mostly used from the main event loop, but also from a thread in
ServerHandler.
This is broken as per specification, and Qt 5.11 seems to finally enforce
that.

To resolve this issue, we promote Database to an instantiable class,
use the created Global context class instance from the event loop,
and a distinct Database instance (and connection) from the ServerHandler
instance (and thread).

@Kissaki Kissaki force-pushed the Kissaki:f/db-thread-access branch from 21b4b3a to 222def7 Jun 18, 2018

@davidebeatrici davidebeatrici merged commit 6195761 into mumble-voip:master Jun 18, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Kissaki Kissaki deleted the Kissaki:f/db-thread-access branch Jul 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.