Singletons #42

Open
m-kuhn opened this Issue Mar 10, 2016 · 13 comments

Projects

None yet

7 participants

@m-kuhn
Member
m-kuhn commented Mar 10, 2016 edited

Singletons have been discussed a lot as something to fix for 3.0.

A possible path forward may be to have one single singleton on top which owns all other "singletons" which would then explicitly be created and managed from this root singleton. This would give us full control over initialization order and allow instancing them from other points as well.

  • QgsApplication (root singleton)
    • QgsGpsConnectionRegistry
    • QgsPaintEffectsRegistry
    • QgsEditorWidgetsRegistry
    • QgsCRSCache / QgsCoordinateTransformCache (is there an internal dependency between them?)
    • QgsColorSchemeRegistry
    • QgsDataItemProviderRegistry
    • QgsProviderRegistry
    • QgsNetworkAccessManager
    • QgsPluginLayerRegistry
    • QgsRasterRendererRegistry
    • QgsRendererV2Registry
    • QgsSvgCache
    • QgsSymbolLayerV2Registry
    • QgsMapLayerActionRegistry
    • QgsMessageLog
  • QgisApp (application specific "singleton")
    • QgsProject
      • QgsMapLayerRegistry
    • QgsAuthManager / QgsCredentials (I guess unlocking should not be controlled globally? Is there an internal dependency between them?)
  • Server
    • QgsConfigCache

Feedback on and changes to the above structure are expected and encouraged!

@nyalldawson

QgsCrsCache has been removed now

@NathanW2
Member

I like this idea. It makes sense to just have a single place for everything and not singletons all over the place.

@wonder-sk thoughts?

@vmora
vmora commented Oct 28, 2016 edited

Since QgisApp is not used in core/, and since I guess you don't intend to introduce the dependency, I understand this change implies that QgsProject::instance() will have to go out of core... do I understand well? If it's the case +1 for me.

@m-kuhn
Member
m-kuhn commented Oct 31, 2016

I think so. My favorite approach would be to drop QgsProject::instance() at some point in the future and instead have a QgisApp::activeProject property. But it will require a lot of time to properly address every usage of QgsProject::instance(), meanwhile with qgis/QGIS-Enhancement-Proposals#76 (comment) at least we can have clearly separated projects and temporarily keep QgsProject::instance() in place.

@luipir
luipir commented Dec 27, 2016
  • QgisApp
    • QgsAuthManager / QgsCredentials (I guess unlocking should not be controlled globally? Is there an internal dependency between them

Observation:
QgsAuthManager uses QgsCredentials just to interface pure virtual method to get MasterPassword:
https://qgis.org/api/qgsauthmanager_8cpp_source.html#l02856

Question:
I'm missing something, please someone can explain me why QgsAuthManager whould be initialised in QgisApp and not in the main singleton manager in QgsApplication?

@m-kuhn
Member
m-kuhn commented Dec 27, 2016

I wasn't quite sure how Credentials and AuthManager work exactly, my proposal is likely wrong.
Question: is there always just one QgsAuthManager? Or can there be multiple different ones, for example in a shared server scenario?

@luipir
luipir commented Dec 27, 2016

Hmmmm... @m-kuhn good question about a shared served scenario. Architecturally is a really pure singleton, but I can't find concurrent access management.
Would be necessary enhancement to use it in a server context. Any opinion @elpaso?

@luipir
luipir commented Dec 27, 2016

IMHO AuthManager in concurrent server context can be simplified stopping and starting server during permission management. BTW this depend if qgis server will evolve to a full featured map server instead of focusing more on the rendering task.

@nyalldawson nyalldawson referenced this issue in qgis/QGIS Dec 27, 2016
Merged

Generic help for QGIS #3911

@nyalldawson

qgis/QGIS#3914 fixes:

  • QgsColorSchemeRegistry
  • QgsDataItemProviderRegistry
  • QgsGPSConnectionRegistry
  • QgsMessageLog
  • QgsPaintEffectRegistry
  • QgsPluginLayerRegistry
  • QgsRasterRendererRegistry
  • QgsRendererRegistry
  • QgsSvgCache
  • QgsSymbolLayerRegistry
@elpaso
elpaso commented Dec 28, 2016

@luipir not sure if that's what you were asking for, but
I think that there should not be any issue with concurrent servers on a single (SQLlite) auth DB: locking is handled by the DB engine itself.
@m-kuhn, I don't see the need of multiple QgsAuthManager instances in the server: even when we are serving multiple project from a single instance (process) of QgsServer, unless we create system to bind a particular project to a particular auth DB, we can stick to the singleton model.

@luipir
luipir commented Dec 28, 2016

@elpaso, AuthManager does not only manage sqlite queries but also internal state and variables... here we can have concurrence problems. Btw I agree that singleton still continue to work in qgis server context. The context premise is where qgis server does not evolve with a web managing interface with multiple managers at the same time. I'm not used in QGIS server, but IMHO A common use case is where qgis desktop is used to manage configuration for the server => single manager. Am I wrong?

@dakcarto
Member

@m-kuhn wrote:

I wasn't quite sure how Credentials and AuthManager work exactly, my proposal is likely wrong.

They only communicate via a signal slot connection at the instance level between singletons. QgisApp is the parent of QgsCredentialDialog::instance. For Server, the master password and database are defined via env variables, so no QgsCredential::instance (or any of its subclasses) is used.

Question: is there always just one QgsAuthManager? Or can there be multiple different ones, for example in a shared server scenario?

The design is for only one, as it is app-relative. Shared server instances or concurrency was never designed for.

I'm looking into a QEP and API refactoring for the auth system, so would like to figure out the best design approach going forward. Can you explain further what you envision a 'shared server scenario' as? Currently, the SQLite qgis-auth.db is app- not project-relative. So, unless multiple projects are served simultaneously by the same QGIS Server FCGI spawned instance, I don't see the issue.

@m-kuhn
Member
m-kuhn commented Jan 28, 2017

Can you explain further what you envision a 'shared server scenario' as?

No vision at all, maybe the server team does have one? I'm happy to keep one per process (and not project) if that is fine for server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment