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

Singletons #42

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

Comments

Projects
None yet
8 participants
@m-kuhn
Member

m-kuhn commented Mar 10, 2016

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

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Jul 26, 2016

Contributor

QgsCrsCache has been removed now

Contributor

nyalldawson commented Jul 26, 2016

QgsCrsCache has been removed now

@NathanW2

This comment has been minimized.

Show comment
Hide comment
@NathanW2

NathanW2 Oct 14, 2016

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?

Member

NathanW2 commented Oct 14, 2016

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

This comment has been minimized.

Show comment
Hide comment
@vmora

vmora Oct 28, 2016

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.

vmora commented Oct 28, 2016

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

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Oct 31, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@luipir

luipir 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?

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

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Dec 27, 2016

Member

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?

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

This comment has been minimized.

Show comment
Hide comment
@luipir

luipir 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 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

This comment has been minimized.

Show comment
Hide comment
@luipir

luipir 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.

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

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Dec 28, 2016

Contributor

qgis/QGIS#3914 fixes:

  • QgsColorSchemeRegistry
  • QgsDataItemProviderRegistry
  • QgsGPSConnectionRegistry
  • QgsMessageLog
  • QgsPaintEffectRegistry
  • QgsPluginLayerRegistry
  • QgsRasterRendererRegistry
  • QgsRendererRegistry
  • QgsSvgCache
  • QgsSymbolLayerRegistry
Contributor

nyalldawson commented Dec 28, 2016

qgis/QGIS#3914 fixes:

  • QgsColorSchemeRegistry
  • QgsDataItemProviderRegistry
  • QgsGPSConnectionRegistry
  • QgsMessageLog
  • QgsPaintEffectRegistry
  • QgsPluginLayerRegistry
  • QgsRasterRendererRegistry
  • QgsRendererRegistry
  • QgsSvgCache
  • QgsSymbolLayerRegistry
@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso 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.

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

This comment has been minimized.

Show comment
Hide comment
@luipir

luipir 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?

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

This comment has been minimized.

Show comment
Hide comment
@dakcarto

dakcarto Jan 27, 2017

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.

Member

dakcarto commented Jan 27, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@m-kuhn

m-kuhn Jan 28, 2017

Member

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.

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.

@haubourg

This comment has been minimized.

Show comment
Hide comment
@haubourg

haubourg May 4, 2017

@pblottiere @rldhont
Concerning the last singletons listed here, what is the current status in most recent code ?

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

Thanks for helping in cleaning up here !

haubourg commented May 4, 2017

@pblottiere @rldhont
Concerning the last singletons listed here, what is the current status in most recent code ?

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

Thanks for helping in cleaning up here !

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson May 7, 2017

Contributor

Here's the current list

App - should become members of QgisApp::instance():

  • QgsCustomization
  • QgsMapLayerStyleGuiUtils
  • QgsMapThemes
  • QgsPluginRegistry

Auth:

  • QgsAuthManager
  • QgsAuthMethodRegistry

Core:

  • QgsCredentials
  • QgsCoordinateTransformCache (needs transparent merging into QgsCoordinateTransform)
  • QgsNetworkAccessManager
  • QgsProject
  • QgsProviderRegistry

Gui:

  • QgsEditorWidgetRegistry
  • QgsLayerTreeEmbeddedWidgetRegistry
  • QgsMapLayerActionRegistry
  • QgsShortcutsManager

Server:

  • QgsConfigCache
  • QgsMSLayerCache
  • QgsServerLogger

The 4 GUI singletons are handled by qgis/QGIS#4514

Contributor

nyalldawson commented May 7, 2017

Here's the current list

App - should become members of QgisApp::instance():

  • QgsCustomization
  • QgsMapLayerStyleGuiUtils
  • QgsMapThemes
  • QgsPluginRegistry

Auth:

  • QgsAuthManager
  • QgsAuthMethodRegistry

Core:

  • QgsCredentials
  • QgsCoordinateTransformCache (needs transparent merging into QgsCoordinateTransform)
  • QgsNetworkAccessManager
  • QgsProject
  • QgsProviderRegistry

Gui:

  • QgsEditorWidgetRegistry
  • QgsLayerTreeEmbeddedWidgetRegistry
  • QgsMapLayerActionRegistry
  • QgsShortcutsManager

Server:

  • QgsConfigCache
  • QgsMSLayerCache
  • QgsServerLogger

The 4 GUI singletons are handled by qgis/QGIS#4514

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Jun 2, 2017

Contributor

No more GUI singletons -

  • QgsEditorWidgetRegistry
  • QgsLayerTreeEmbeddedWidgetRegistry
  • QgsMapLayerActionRegistry
  • QgsShortcutsManager

Can be removed from the list

Contributor

nyalldawson commented Jun 2, 2017

No more GUI singletons -

  • QgsEditorWidgetRegistry
  • QgsLayerTreeEmbeddedWidgetRegistry
  • QgsMapLayerActionRegistry
  • QgsShortcutsManager

Can be removed from the list

@elpaso

This comment has been minimized.

Show comment
Hide comment
@elpaso

elpaso Nov 3, 2017

QgsAuthManager can be removed from the list

elpaso commented Nov 3, 2017

QgsAuthManager can be removed from the list

@nyalldawson

This comment has been minimized.

Show comment
Hide comment
@nyalldawson

nyalldawson Nov 6, 2017

Contributor

qgis/QGIS#5535 removes QgsCoordinateTransformCache singleton

Contributor

nyalldawson commented Nov 6, 2017

qgis/QGIS#5535 removes QgsCoordinateTransformCache singleton

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