Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Make sure that we close db connections opened during init #3764

Merged
merged 4 commits into from Aug 30, 2018

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Aug 28, 2018

We should explicitly close any db connections we open, because failing to do so
can block other transactions as per
#3682.

Let's also try to factor out some of the boilerplate by having server classes
define their datastore class rather than duplicating the whole of setup.

We should explicitly close any db connections we open, because failing to do so
can block other transactions as per
#3682.

Let's also try to factor out some of the boilerplate by having server classes
define their datastore class rather than duplicating the whole of `setup`.
@richvdh richvdh requested a review from a team August 28, 2018 12:43
@erikjohnston
Copy link
Member

Is it worth using abc package and a DATA_STORE = abc.abstractproperty()?

@richvdh
Copy link
Member Author

richvdh commented Aug 28, 2018

Is it worth using abc package and a DATA_STORE = abc.abstractproperty()?

AIUI that would mean subclasses would have to implement a DATA_STORE() getter, which sounds boilerplatey

@erikjohnston
Copy link
Member

AIUI that would mean subclasses would have to implement a DATA_STORE() getter, which sounds boilerplatey

Nope, we do it for the replication HTTP classes, e.g.: https://github.com/matrix-org/synapse/blob/master/synapse/replication/http/_base.py#L70 vs https://github.com/matrix-org/synapse/blob/master/synapse/replication/http/federation.py#L47

This gives clearer messages when someone gets it wrong
@richvdh richvdh merged commit 475253a into develop Aug 30, 2018
@richvdh richvdh deleted the rav/close_db_conn_after_init branch August 30, 2018 09:47
@richvdh richvdh restored the rav/close_db_conn_after_init branch September 6, 2018 12:00
@hawkowl hawkowl deleted the rav/close_db_conn_after_init branch September 20, 2018 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants