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

avoid recursive exec() of QgsCredentials #9333

Merged
merged 1 commit into from Mar 23, 2019

Conversation

jef-n
Copy link
Member

@jef-n jef-n commented Mar 3, 2019

followup c9e7616 - mutex didn't only guard the cache, but also the dialog

@nyalldawson
Copy link
Collaborator

@jef-n I don't understand the need for this -- and I believe it's going to bring back the original deadlock which my fix was designed to avoid.

Why would the dialog need a mutex? it's always run in the main thread

@jef-n jef-n force-pushed the avoid-recursive-credential-requests branch from d5341ee to 06ec3af Compare March 3, 2019 23:37
@jef-n
Copy link
Member Author

jef-n commented Mar 3, 2019

If there are multiple layers eg. from the same postgres database requiring the same credentials, there will be multiple credential requests in parallel - but the second and following are canceled on exec() because the first request is still executing.

@nyalldawson
Copy link
Collaborator

Can you please test to make sure #20826 hasn't been revived?

@@ -1589,6 +1589,10 @@ QgisApp::~QgisApp()
qDeleteAll( mCustomDropHandlers );
qDeleteAll( mCustomLayoutDropHandlers );

// replace gui based network access managers with failing only ones
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part should not be revived

@jef-n jef-n force-pushed the avoid-recursive-credential-requests branch from 06ec3af to 6f634bb Compare March 4, 2019 12:43
@jef-n jef-n self-assigned this Mar 4, 2019
@elpaso
Copy link
Contributor

elpaso commented Mar 6, 2019

Please have a look to my comments on https://issues.qgis.org/issues/21461

@giedrioks26
Copy link

this is related https://issues.qgis.org/issues/21582?

@jef-n
Copy link
Member Author

jef-n commented Mar 15, 2019

this is related https://issues.qgis.org/issues/21582?

yes. The reverted change cause this

Partly reverts c9e7616, which removed the synchronizatiion of
credential requests (eg. in a project that has multiple layers from the
same postgresql database without credentials) and led to multiple
concurrent requests for the same credentials.

Some of which were silently discarded, when events processed in the
dialogs exec() event loop tried to reinvoke the dialog and caused
invalid layers.

Authentications caused by network requests can still cause this.

The credential cache is now guarded by a separate mutex.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants