More keychain fixes #1494
More keychain fixes #1494
Conversation
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
Sources/Plasma/FeatureLib/pfPasswordStore/pfPasswordStore_Apple.cpp
Outdated
Show resolved
Hide resolved
| dispatch_async(_keychainQueue, ^{ | ||
| pfPasswordStore* store = pfPasswordStore::Instance(); | ||
| if (self.rememberPassword) | ||
| store->SetPassword(username, password); | ||
| else | ||
| store->SetPassword(username, ST::string()); | ||
| }); |
There was a problem hiding this comment.
If I understand correctly, this is enqueuing the pfPasswordStore function call to happen on a secondary thread. The goal of pfPasswordStore::Instance() is to return a platform agnostic password store. The expectation is that all of the password stores have the same interface. The interface of the Win32 and Linux password stores is thread-agnostic. Therefore, it is important that the Apple password store be thread-agnostic as well. Thus, the worker thread fiddling needs to be done in pfPasswordStore_APPLE so that the interfaces of all password stores match exactly.
There was a problem hiding this comment.
That might be complicated in the case of retrieving the password though (you could probably do something with std::promise and changing the interface to return a std::future<ST::string> but that's a bit complicated...) 😕
There was a problem hiding this comment.
For extra fun - reading the password seems fine? It's the writing that is the issue. The dispatch threading could be moved to the password store class itself (or be replaced by C++ threading but that's a longer effort.)
FWIW - I don't know why the debugger throws this warning at runtime. They warn it could stall the main thread but don't say why. We trigger the keychain unlock dialog (if needed) at launch. It could be that if the user waits long enough, the keychain will lock, setting the password will need a dialog, and the main thread will be held.
There was a problem hiding this comment.
Checking back in again - should re-arranging the underlying API to handle threading with promises be part of this PR? I can make the change - but I just want to make sure that's a direction we're all in agreement on.
There was a problem hiding this comment.
I think it would be nice if we could get away with not using a defferred (read std::promise) for reading the password. IMO, it's ok if the main thread stalls there - I'm assuming that macOS has stolen the thread to prompt the user for some interaction. I would prefer to see the password saving dispatched inside pfPasswordStore, though.
There was a problem hiding this comment.
Without knowing more about Apple's definition of blocking - I agree that this probably is fine on the main thread.
However - and this is what caused this PR - Xcode flags a runtime error/warning when we make this call off the main thread. So this issue is currently leading to us not getting clean runs out of Xcode.
colincornaby
force-pushed
the
more-keychain-fixes
branch
from
f004031
to
f032344
Compare
|
I think this is stuck on the "should asyncronisity be baked in at an API level" question. |
colincornaby
force-pushed
the
more-keychain-fixes
branch
from
f032344
to
a8bdbe5
Compare
|
I'm circling back to this PR as I was running into some of these issues when testing deploying a server the other day. I think where we got stuck is the main thread issues. Apple does not want us using the main thread - to the point where this gets raised as a runtime warning within Xcode. I'm not entirely clear on why not calling on the main thread is preferred. I can continue to investigate. As stated in the previous comment chain, I think we'd maybe be fine using the main thread to store passwords. But it would be nice to know why Apple thinks we shouldn't be safe. And it would be nice to not have the runtime warning. |
Minor fixes