| *** zbenjamin is now known as Guest60515 | 02:37 | |
| *** zbenjamin_ is now known as zbenjamin | 02:37 | |
| *** Helle is now known as Guest89912 | 04:26 | |
| *** frinring_ is now known as frinring | 04:50 | |
| dcaliste | chriadam: great the CI issue is fixed for messagingframework. Thanks. | 08:18 |
|---|---|---|
| chriadam | dcaliste: "fixed" might be a bit optimistic :-P | 08:19 |
| chriadam | sledgehammered into submission :-P | 08:19 |
| dcaliste | Ok, bypassed then! | 08:19 |
| dcaliste | chriadam: 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 |
| chriadam | sure | 08:22 |
| chriadam | I haven't had a chance to read your recent comments or check the PR | 08:22 |
| dcaliste | Looking at it, I see to major blocking points: | 08:22 |
| chriadam | my 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 |
| dcaliste | looking for the code lines… | 08:27 |
| chriadam | oh, no problem | 08:27 |
| dcaliste | see requestqueue.cpp:73 | 08:29 |
| dcaliste | The 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 |
| dcaliste | I'm not sure it's a good idea to make this simple structure a QObject… | 08:30 |
| dcaliste | That'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 |
| dcaliste | But, 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 |
| dcaliste | So, 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 |
| dcaliste | chriadam: 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 |
| chriadam | so, 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 |
| chriadam | thus, you should be able to safely call: QtConcurrent::run(threadForPlugin(plugin), Plugin::CancelRequest, QVariantList() << requestId); | 08:35 |
| chriadam | internally, 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 |
| dcaliste | Yes, 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 |
| chriadam | why is a mutex needed? | 08:37 |
| dcaliste | Because we keep an association QHash(cookie, Responses). | 08:37 |
| dcaliste | So if two thread need to access this table, we're screwed without mutex. | 08:38 |
| chriadam | I agree, but two threads should never need to access that table | 08:38 |
| dcaliste | But 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 |
| chriadam | it should be the case already | 08:39 |
| chriadam | every plugin has exactly one thread associated with it | 08:39 |
| dcaliste | If so, yeah, I agree no need for the mutex. | 08:39 |
| dcaliste | And the implementation of cancelFoo() is much simpler then. | 08:39 |
| chriadam | double 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 |
| chriadam | for the first issue: let me check requestqueue.cpp:73 | 08:41 |
| dcaliste | Ok, 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 |
| dcaliste | For 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 |
| dcaliste | The 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 |
| chriadam | the 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 |
| chriadam | well, you could forward it via a lambda | 08:46 |
| chriadam | uh. maybe not for dbus signals. hrm. | 08:46 |
| dcaliste | Yes, you see my concern… | 08:46 |
| chriadam | indeed | 08:47 |
| dcaliste | Imho the best place is in controller but I cannot figure out how to know the connection from the disconnect handler. | 08:47 |
| dcaliste | The second best place is when the RequestData is created and the connection associated there, but it's not a QObject… | 08:47 |
| chriadam | FWIW you could make the RequestData a QObject if necessary. that would have some perf impact, however.. | 08:49 |
| dcaliste | Yeah, 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 |
| dcaliste | But 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 |
| chriadam | we hope :-P | 08:54 |
| chriadam | but I'm pretty sure | 08:54 |
| chriadam | https://github.com/sailfishos/sailfish-secrets/blob/master/daemon/controller.cpp#L122 | 08:55 |
| chriadam | hrm. perhaps we only do that for secrets+crypto plugins, not for auth plugins? will check... | 08:56 |
| dcaliste | Auth is a secret plugin, so it should be fine. | 09:00 |
| dcaliste | I'll check with some debug but great. I don't like to handle mutex… | 09:00 |
| chriadam | hrm, seems like we may do authentication requests in the main (gui) thread, rather than either the secrets or crypto thread. see e.g. https://github.com/sailfishos/sailfish-secrets/blob/master/daemon/SecretsImpl/secretsrequestprocessor.cpp#L783 | 09:02 |
| chriadam | either way, mutex should not be required, but that's interesting... | 09:02 |
| dcaliste | Ah, 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 |
| dcaliste | Ah, no you're right, they are from the main thread… | 09:05 |
| chriadam | main | 09:05 |
| dcaliste | The 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 |
| chriadam | that should hopefully never happen, though. the request queue lives in the main thread | 09:32 |
| chriadam | so any requests from clients will be handled on the main thread | 09:32 |
| chriadam | and any disconnection of clients from the dbus connection, will be handled on the main thread | 09:32 |
| chriadam | unless I'm misunderstanding | 09:32 |
| dcaliste | chriadam: 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 |
| chriadam | thanks! | 09:41 |
| chriadam | have a great weekend :-) | 09:41 |
| dcaliste | Thank you, have a nice one too. | 09:44 |
| m4g0g | hello everybody | 12:54 |
| m4g0g | I am in process of creating google photo account support in SailfishOS | 12:55 |
| m4g0g | I 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 |
| m4g0g | As I understand from code: filteredData.remove(SSO_ACCESS_CONTROL_TOKENS); (https://git.merproject.org/mer-core/libsignon-qt5/blob/master/libsignon/src/signond/signonsessioncore.cpp#L640) | 12:57 |
| m4g0g | username, 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 Guest78787 | 15:14 | |
| *** Helle is now known as Guest99467 | 16:18 | |
| *** Helle is now known as Guest3455 | 19:03 | |
Generated by irclog2html.py 2.17.1 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!