New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 739897 link

Starred by 3 users
Status: WontFix
Owner: ----
Closed: Jul 2017
Cc:
dvadym@chromium.org
jam@chromium.org
cfroussios@chromium.org
█ mdm@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Task



Sign in to add a comment

Can we drop Wait() from NativeBackendGnome?

Project Member Reported by vabr@chromium.org, Jul 6 2017 
Methods like NativeBackendGnome::AddLogin currently run on the background thread, then post a task for the GnomeKeyring glue to the main thread and use a WaitableEvent to block the background thread until the response from main arrives.

Is there a reason this needs to be synchronous, or can we split AddLogin into the part before and after the post to the main thread, and pass the second part as a callback to the task on the main thread?

cfroussios@ and dvadym@, you have both worked on this code at some point, what is your opinion?

Comment 1 by dvadym@chromium.org, Jul 7 2017 

The main reason is that NativeBackendGnome::AddLogin and others are supposed to be synchronous in all call sites. So it would require quite substantial changes to make code synchronous in all call places. And what's even worse AddLogin is a virtual function, and all other backends have it, so it would require changes in all backends.

So it's possible to make it async, but it will require quite a lot of work and would cost adding substantial complexity in PasswordStore. That's why I decided to implement NativeBackendLibsecret 2 years ago, which doesn't have this problems. And it fixed problems related to NativeBackendGnome for all users who have LibSecret installed. And taking in consideration that gnome keyring library has been deprecated since at least 2014, I believe we should leave NativeBackendGnome as is.

Comment 2 by cfroussios@chromium.org, Jul 7 2017 

All of GnomeKeyring, Libsecret and dbus (which we use for KWallet) have async variants for calls.

I'm not aware why they were not used initially. Today, my argument would be that it's not worth the effort to refactor the backends and that we should work on deprecating them, removing the issue altogether.

Comment 3 by vabr@chromium.org, Jul 7 2017

Status: WontFix (was: Available)
Thanks both of you for the helpful input.

I agree that refactoring all of this to get rid of the Wait() call would be lost time, which we can better invest towards removing the backends altogether.

I'll close this as Wontfix, and use the bug as a documentation of the decision in the code.

Comment 4 by vabr@chromium.org, Jul 7 2017

Cc: mdm@chromium.org jam@chromium.org
 Issue 125331  has been merged into this issue.

Comment 5 by vabr@chromium.org, Jul 7 2017 

For additional reference, the backend deprecation is tracked under bug 571003.

Comment 6 by vabr@chromium.org, Nov 29

Cc: -vabr@chromium.org
►

Sign in to add a comment