Friday, 2019-03-15

*** zbenjamin is now known as Guest6051502:37
*** zbenjamin_ is now known as zbenjamin02:37
*** Helle is now known as Guest8991204:26
*** frinring_ is now known as frinring04:50
dcalistechriadam: great the CI issue is fixed for messagingframework. Thanks.08:18
chriadamdcaliste: "fixed" might be a bit optimistic :-P08:19
chriadamsledgehammered into submission :-P08:19
dcalisteOk, bypassed then!08:19
dcalistechriadam: If you have five minutes, may we discuss a bit the cancellation issue in Secrets ? But I understand it's late, we can do this another day also…08:21
chriadamI haven't had a chance to read your recent comments or check the PR08:22
dcalisteLooking at it, I see to major blocking points:08:22
chriadammy IRC might have just messed up, I didn't what you wrote (the major blocking points)08:26
dcaliste* the disconnection signal should be connected for every request, which makes sense to me, but the request is there a simple structure not an object.08:26
dcaliste(I'm writing and discussing with a collegue at the same time, sorry)08:27
dcalistelooking for the code lines…08:27
chriadamoh, no problem08:27
dcalistesee requestqueue.cpp:7308:29
dcalisteThe connection is set there and the disconnection signal should be attached here I think. But data is a plain structure, not a QObject.08:29
dcalisteI'm not sure it's a good idea to make this simple structure a QObject…08:30
dcalisteThat's my first point, the second is…08:30
dcaliste* there should be a cancelAuthentication(uint callerId, quint64 requestId) and cancelUserInteraction(idem) interface in plugin.08:31
dcalisteBut, implementing it passwordplugin for instance raise some unexpected issues related to threads: the beginAuthentication() as been run in a Concurrent thread, but the cancel call will come from another.08:32
dcalisteSo, I need to add a Mutex there on the QHash of the stored authentications (like m_polkitResponses or m_sessionAgent.responses for instance) and protect all accesses to these QHash with the mutex.08:33
dcalistechriadam: I let you think about these and tell me if it makes sense to continue in that direction. Something that I initially thought as a minor fix is transforming into a major modification, so before going further and finalise the implementation, I would like your opinion on the matter.08:34
chriadamso, the design is such that every plugin should be associated with exactly one thread over its lifetime - only its constructor should be run in the main thread.08:34
chriadamthus, you should be able to safely call: QtConcurrent::run(threadForPlugin(plugin), Plugin::CancelRequest, QVariantList() << requestId);08:35
chriadaminternally, the authentication plugin flow should be asynchronous, so when it receives that cancelRequest call, it should be able to do ... whatever it needs to do internally to cancel the ongoing request.08:36
chriadam(I agree that the cancelAuthentication + cancelUserInteraction methods need to be added)08:36
dcalisteYes, that was my concern and frustration: the underlying impl for password auth is asynchronous by design, but I need some mutex for the internal QHash…08:36
chriadamwhy is a mutex needed?08:37
dcalisteBecause we keep an association QHash(cookie, Responses).08:37
dcalisteSo if two thread need to access this table, we're screwed without mutex.08:38
chriadamI agree, but two threads should never need to access that table08:38
dcalisteBut I'll look at your suggestion to keep track of the thread and run the action in the same thread. Or is it the case already ?08:38
chriadamit should be the case already08:39
chriadamevery plugin has exactly one thread associated with it08:39
dcalisteIf so, yeah, I agree no need for the mutex.08:39
dcalisteAnd the implementation of cancelFoo() is much simpler then.08:39
chriadamdouble check because my memory might be wrong ;-) but (aside from ctor) every method of any given plugin should only ever be invoked from some specific thread, via QtConcurrent.08:40
chriadamfor the first issue: let me check requestqueue.cpp:7308:41
dcalisteOk, I didn't notice this (and I did look for it neither), but that's great, simplies things a lot in cancelFoo() implementation.08:41
dcalisteFor the first issue, I point this line, because the disconnection should have access to the request to be able to cancel it, and it's the place where we have remotePid and requestId, imho.08:43
dcalisteThe other solution would be to add the disconnect signal in controller.cpp:274 and in the handler to look for the request with the right connection, but I don't see how we can know the connection from the disconnect signal…08:45
chriadamthe case I was thinking about is if the client disconnects entirely.  so in handleClientConnection(), connect the Disconnected signal of the connection to a slot (cancelOngoingRequests()).  would need to store a hash of connection to ongoing requests, and housekeep that properly, though :-/08:46
chriadamwell, you could forward it via a lambda08:46
chriadamuh.  maybe not for dbus signals.  hrm.08:46
dcalisteYes, you see my concern…08:46
dcalisteImho the best place is in controller but I cannot figure out how to know the connection from the disconnect handler.08:47
dcalisteThe second best place is when the RequestData is created and the connection associated there, but it's not a QObject…08:47
chriadamFWIW you could make the RequestData a QObject if necessary.  that would have some perf impact, however..08:49
dcalisteYeah, that's what I wanted to avoid, to intrusive in my opinion, higher risk of impacting other things… For the moment, I've left it untouched and I'm preparing the plugin extension implementation for cancelFoo(). Then, I'll see where this should be called from and going reverse like that, see if it's the only reasonable solution.08:52
dcalisteBut that's why I wanted also to discuss it a bit before proceeding. You already save me the mutex mess in impl. Thanks.08:53
chriadamwe hope :-P08:54
chriadambut I'm pretty sure08:54
chriadamhrm.  perhaps we only do that for secrets+crypto plugins, not for auth plugins?  will check...08:56
dcalisteAuth is a secret plugin, so it should be fine.09:00
dcalisteI'll check with some debug but great. I don't like to handle mutex…09:00
chriadamhrm, seems like we may do authentication requests in the main (gui) thread, rather than either the secrets or crypto thread.  see e.g.
chriadameither way, mutex should not be required, but that's interesting...09:02
dcalisteAh, yeah, that's true. But these functions from secretreuestprocessor, are they called indeed from the main thread ? Or are they already in the QtConcurrent one ?09:04
dcalisteAh, no you're right, they are from the main thread…09:05
dcalisteThe fact that this interaction request is done in the main thread will creates issue if we try to cancel it from a QtConcurrent thread…09:11
chriadamthat should hopefully never happen, though.  the request queue lives in the main thread09:32
chriadamso any requests from clients will be handled on the main thread09:32
chriadamand any disconnection of clients from the dbus connection, will be handled on the main thread09:32
chriadamunless I'm misunderstanding09:32
dcalistechriadam: I'm not sure, depends how the call to cancelFoo() is implemented. But thank you for the discussion, it helped me to see where to go. We can continue this hopefully on implementation next week if I have time to convert all this into code in between ;)09:40
chriadamhave a great weekend :-)09:41
dcalisteThank you, have a nice one too.09:44
m4g0ghello everybody12:54
m4g0gI am in process of creating google photo account support in SailfishOS12:55
m4g0gI have next question: my account was added successfully and I got accessToken from google, but where is this access token stored, because I need it to use in transfer engine plugin?12:56
m4g0gAs I understand from code: filteredData.remove(SSO_ACCESS_CONTROL_TOKENS); (
m4g0gusername, password and tokens don't store at all. What the right process of getting token? Should I every time create authsession and ask about new access token?12:58
*** Helle is now known as Guest7878715:14
*** Helle is now known as Guest9946716:18
*** Helle is now known as Guest345519:03

Generated by 2.17.1 by Marius Gedminas - find it at!