search

flatpak / xdg-dbus-proxy

GNU Lesser General Public License v2.1
57 stars 21 forks source link

auth: Support sd-bus authentication pipelining #48

Closed evan-a-a closed 1 month ago

evan-a-a commented 1 year ago

Since sd-dbus clients send the BEGIN before receiving OK, ensure that we look for both to come across the bus in either order.

Fixes https://github.com/flatpak/xdg-dbus-proxy/issues/21

evan-a-a commented 1 year ago

@alexlarsson or @smcv, can you take a look at this? I believe it solves the concerns present in #26. This change is important for https://github.com/flathub/com.microsoft.Edge/pull/252 and other applications that rely on sd-bus.

alexlarsson commented 1 year ago

It feels a bit lame to read a byte at a time. Thats quite a lot of syscalls... Did you look at how this affects the time spent on authentication?

Anyway, i would like @smcv to look at this too. He knows the dbus stuff best.

alexlarsson commented 1 year ago

The reading of one line a time will also cause the writing to the proxy client to be split up, so the performance is not just in the number of read calls. It will become multiple "network packets" too.

evan-a-a commented 1 year ago

It feels a bit lame to read a byte at a time. Thats quite a lot of syscalls... Did you look at how this affects the time spent on authentication? The reading of one line a time will also cause the writing to the proxy client to be split up, so the performance is not just in the number of read calls. It will become multiple "network packets" too.

I agree, it's quite lame. Handling the over-read with the extra buffer management seems to be a "less clean" option to me. I actually based this on how gdbus' server code fixes the handling of sd-bus clients. In https://github.com/GNOME/glib/commit/8f02681f6e2130c52f27c1edb4febb1443e97d94, they modified their code to only read one byte at a time, which prevents the over-read case. I didn't benchmark exactly how much this slows down the auth process, but I didn't notice any responsiveness issues in the one sd-bus client I tested. I can try to set up a test harness to collect some timing info.

evan-a-a commented 1 year ago

Actually, after looking at the reference implementation, I have a better idea about how to tackle this while avoiding the single byte reads. I will work on that improvement.

bilelmoussaoui commented 1 month ago

A variant of this has been merged in #54