Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dynamic client connection class #2351

Closed
totaam opened this issue Jul 2, 2019 · 6 comments
Closed

dynamic client connection class #2351

totaam opened this issue Jul 2, 2019 · 6 comments
Labels

Comments

@totaam
Copy link
Collaborator

totaam commented Jul 2, 2019

Taking #1838 one step further: when the client disables a feature completely (ie: clipboard) or when the client is not a UI client (ie: #2348) then we can generate a custom client class without those base classes.

get_client_connection_class needs to be made dynamic.
We'll need a map from flags to mixins, ie:

 "clipboard" : ClipboardConnection,
 "webcam" : WebcamMixin,
etc..
@totaam
Copy link
Collaborator Author

totaam commented Jul 2, 2019

Preparatory work in r23081.

@totaam
Copy link
Collaborator Author

totaam commented Jul 8, 2019

This will be much easier to implement after dropping python2 support. (v4)

@totaam
Copy link
Collaborator Author

totaam commented Jan 3, 2020

2020-01-03 11:30:56: antoine uploaded file skip-clientconnection-mixins.patch (15.7 KiB)

implementation - causes server hangs on control-c

@totaam
Copy link
Collaborator Author

totaam commented Jan 5, 2020

No idea why, but re-doing the change step by step no longer crashes and so the code has been merged in r24907.
The only mixin we currently skip is the mmap mixin.

Still TODO:

  • add the is_needed check to more mixins
  • try to minimize module level imports so that the is_needed check does not cost too much
  • in some cases, add a new flag so we can distinguish between something that is available but disabled from something that is not available (ie: av-sync could start turned off and be enabled via a control command, clipboard)

@totaam
Copy link
Collaborator Author

totaam commented Jan 5, 2020

Much improved in r24910, see commit message for details. With minor updates in r24911 + r24912 + r24913 + r24914.
Minimal backport to help the server figure out what the client is capable of: 24915.
With a regular client and xpra top connected:

$ xpra info | grep ".modules"
client.1.modules=('Client', 'ClientInfo', 'Clipboard', 'Audio', 'Webcam', 'FilePrint', 'MMAP', 'Input', 'DBUS', 'NetworkState', 'ClientDisplay', 'Windows', 'Encodings', 'AVSync', 'Idle')
client.modules=('Client', 'ClientInfo', 'Webcam', 'Input', 'DBUS', 'NetworkState', 'ClientDisplay', 'Idle')

Apart from the stated goals of not loading code we don't need (reducing attack surface, reducing memory usage, etc), there are additional benefits: we are forced to remove cross-mixin dependencies and this reduces contention on shared resources. ie: a client which has disabled the clipboard will make it easier for other users.

Some caveats.

  • av-sync: older clients will send "av-sync" properties even when disabled... so with those we enable the mixin even when not needed - could backport this trivial fix
  • display mixin doesn't have any clear on-off properties
  • input mixin needs disentangling
  • webcam lacked a client capability attribute... so we should add a separate client flag to workaround that (ie: "webcam.mixin.caps")

Still TODO: test all possible combinations (one of the unit tests can do that - just disabled by default because it takes so long)

@totaam
Copy link
Collaborator Author

totaam commented Feb 6, 2020

Unless needed skip those mixins:

This allows the "top" client to load only the bare minimum:

client.modules=('Client', 'ClientInfo', 'ClientDisplay')

This will do for v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant