⚡️ zb: Pipeline client-side handshake as much as possible #683
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zeenix
force-pushed
the
pipeline-handshake
branch
2 times, most recently
from
80fcf3f to
4d1bcef
Compare
|
@kas (I assume you're this guy) Hi there, seems the gdbus doesn't really correctly do handshake and pipelining handshake breaks there. While I was more than happy to add the support for that but if supporting the special gdbus session bus means we don't get to do smart things in zbus, I'd rather remove the support (or be OK with it being broken until gdbus is fixed). |
zeenix
force-pushed
the
pipeline-handshake
branch
2 times, most recently
from
d8757f5 to
48e3cae
Compare
They're already run in the setup step before.
zeenix
force-pushed
the
pipeline-handshake
branch
from
48e3cae to
ce2e5ab
Compare
Oh never mind. The issue seems reproducible with dbus-daemon as well. I need to make the pipelining be able handle disconnection from the server, as well. |
zeenix
force-pushed
the
pipeline-handshake
branch
2 times, most recently
from
0799845 to
9694445
Compare
zeenix
changed the title
zeenix
force-pushed
the
pipeline-handshake
branch
from
9694445 to
c9eae0d
Compare
zeenix
force-pushed
the
pipeline-handshake
branch
4 times, most recently
from
3fe0618 to
25307e6
Compare
|
Seems I was right about GDBus being unable to handle pipelining. It requires BEGIN command to be sent separately after client receive OK on the authentication. Fortunately, the workaround is not too dirty. |
We now send `DATA`, `NEGOTIATE_UNIX_FD` and `BEGIN` commands at once. This reduces latency and hence improves performance. It would have been the best to send all the commands at the same time, but seems dbus-daemon kick us out if we send multiple commands after it rejects the auth command. It wants us to negotiate the auth first. We could in theory just reconnect but that would require some achitectural changes and rethinking the code. Moreover cookie auth requires a round-trip to the server to get the cookie context first so we can't fully pipeline that one. Furthermore, gdbus is not able to handle `BEGIN` sent along with `DATA`, so we have to send `BEGIN` separately when gdbus feature is enabled on Windows. The server side is not possible to pipeline as it the server just responds to commands from the client and it can not assume that clients will always pipeline all commands. Fixes dbus2#493.
Otherwise it messes up the logging output.
If a received command from the client is unknown or invalid in the current context, we shouldn't completely error out but rather send an error and continue as if nothing happened.
If the client sends a `CANCEL` command during a handshake, the server must move on to the handshake method available (if any). The exact wording in the spec: > On receiving the CANCEL command, the server must send a REJECTED > command and abort the current authentication exchange.
zeenix
force-pushed
the
pipeline-handshake
branch
from
25307e6 to
73797b7
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We now send
DATA,NEGOTIATE_UNIX_FDandBEGINcommands at once. This reduces latency and hence improves performance.It would have been the best to send all the commands at the same time, but seems dbus-daemon kick us out if we send multiple commands after it rejects the auth command. It wants us to negotiate the auth first. We could in theory just reconnect but that would require some architectural changes and rethinking the code.
Moreover cookie auth requires a round-trip to the server to get the cookie context first so we can't fully pipeline that one.
The server side is not possible to pipeline as it the server just responds to commands from the client and it can not assume that clients will always pipeline all commands.
Fixes #493.