Fix deadlock in oath2 authentication #59557
Conversation
The internal copy of the o2 library has customisations and API changes which are required for QGIS... as a result it hasn't been possible to build with the non-internal library version for years. Remove all this dead/unused code.
We only ever create one instance of a QgsAuthMethod, so there's no need for this member to be static
This is being accessed across multiple threads, so we need to protect access accordingly
We do not need to use deleteLater here for QgsAuthOAuth2Config, so we can considerably simplify the logic
And make ownership clear by setting parent to the QgsO2 parent
We have to take care that we don't directly create QgsO2 objects on the thread where the authentication request is occurring, as QgsO2 objects are long running but the thread requesting authentication may be short-lived. If we create QgsO2 objects on a short-running thread, then when we later try to authenticate using the same oauth2 authentication method we will get a deadlock, as the O2 reply server will no longer have an active thread or an event loop running. This was especially evident in browser items which use oauth2 authentication. Since these are usually populated using very short-life threads, we'd often hit this situation: 1. The connection would be expanded. Browser would create a thread to populate the connection. The oauth2 objects would then be created on this same thread, and everything would initially work OK. The user could complete the authentication since the browser population thread was blocked until this was done. 2. The browser item got populated, and then the thread populating it was destroyed 3. Later, something else would request authentication using the same oauth2 config. This may be eg the user expanding out a different folder on the browser connection. 4. If the oauth2 token had expired, then we'd try to refresh it. But this relied on event-based logic, and the event loop for the QgsO2 object was no longer around to handle this. The authentication request would dead-lock. Fix this by ensuring that all QgsO2 objects are created and run on an instance of a new QgsOAuth2Factory QThread. This thread is created on demand, and will then exist for the life of the QGIS session. This ensures that all QgsO2 objects will run on a thread with an application-long lifetime, so there's no issue if they later require event-loop based logic (such as token refresh)
We need to ensure that the link method is ONLY ever called from the QgsO2 object's thread, because it involves creation of child items and that is explicitly prohibited from external threads by Qt.
src/auth/oauth2/CMakeLists.txt
Outdated
|
|
||
| target_include_directories(authmethod_oauth2_a PRIVATE | ||
| ${CMAKE_SOURCE_DIR}/external/qjsonwrapper | ||
| ${CMAKE_SOURCE_DIR}/external/o2/src |
There was a problem hiding this comment.
With this change we accept the fact that we are adopting a forked version of o2.
This makes it different from other external "convenience" libraries to overcome the lack of these in some distributions.
I would suggest we move this out of the external folder into src/auth/oauth2, as all the cmake logic to build this library also sits in here.
Ideally I would also suggest to namespace the code to avoid symbol collision when linking to other libraries that depend on an upstream version of o2. (I've gone through some pain for poly2tri and similar libs in the past).
There was a problem hiding this comment.
With this change we accept the fact that we are adopting a forked version of o2.
I don't see any change in the situation here. This was all dead code, unused by anyway since the introduction of the method.
Instead of abandoning any hope of a maintained upstream, I'd prefer to leave things as they are BUT fork the library repo to QGIS/o2, and then maintain our patchset on there. At least then we'd be making a semi-maintained version of the library available for others to use...
There was a problem hiding this comment.
Yep, that would be a good move indeed.
With vcpkg builds, I would like to build this as an external port and disable WITH_INTERNAL_O2 (this will improve the caching of the dependency and speed up the build).
There was a problem hiding this comment.
@m-kuhn thanks for creating https://github.com/qgis/o2 -- I've just submitted a bunch of pull requests adding downstream changes onto that repo.
When those are merged I'll do a bit of modernization on the library (eg remove gross old style connects), and then resync our external copy.
I'm thinking I'll drop 81d221c just in case there's anything useful in there for you. I suspect you'll need to rewrite a lot of that anyway since it was never used, but maybe there's something there 😆
🪟 Windows buildsDownload Windows builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
We have to take care that we don't directly create QgsO2 objects on the thread where the authentication request is occurring,
as QgsO2 objects are long running but the thread requesting authentication may be short-lived. If we create QgsO2 objects
on a short-running thread, then when we later try to authenticate using the same oauth2 authentication method we
will get a deadlock, as the O2 reply server will no longer have an active thread or an event loop running.
This was especially evident in browser items which use oauth2 authentication. Since these are usually populated using very
short-life threads, we'd often hit this situation:
different folder on the browser connection.
QgsO2 object was no longer around to handle this. The authentication request would dead-lock.
Fix this by ensuring that all QgsO2 objects are created and run on an instance of a new QgsOAuth2Factory QThread. This thread is created on demand, and will then exist for the life of the QGIS session.
This ensures that all QgsO2 objects will run on a thread with an application-long lifetime, so there's no issue if they later
require event-loop based logic (such as token refresh)
Funded by RGD Savoie Mont Blanc