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

Make properly Send safe #156

Merged
merged 8 commits into from Jan 25, 2020
Merged

Make properly Send safe #156

merged 8 commits into from Jan 25, 2020

Conversation

wez
Copy link
Collaborator

@wez wez commented Jan 19, 2020

In earlier iterations I accidentally removed Send from Session and then
later restored it in an unsafe way. This commit restructures the
bindings so that each of the objects holds a reference to the
appropriate thing to keep everything alive safely, without awkward
lifetimes to deal with.

The key to this is that the underlying Session is tracked by an
Arc<Mutex<>>, with the related objects ensuring that they lock this
before they call into the underlying API.

In order to make this work, I've had to adjust the API around iterating
both known hosts and agent identities: previously these would iterate
over internal references but with this shift there isn't a reasonable
way to make that safe. The strategy is instead to return a copy of the
host/identity data and then later look up the associated raw pointer
when needed. The purist in me feels that the copy feels slightly
wasteful, but the realist justifies this with the observation that the
cardinality of both known hosts and identities is typically small enough
that the cost of this is in the noise compared to actually doing the
crypto+network ops.

I've removed a couple of error code related helpers from some of
the objects: those were really internal APIs and were redundant
with methods exported by the Error type anyway.

Fixes: #154
Refs: #137

In earlier iterations I accidentally removed Send from Session and then
later restored it in an unsafe way.  This commit restructures the
bindings so that each of the objects holds a reference to the
appropriate thing to keep everything alive safely, without awkward
lifetimes to deal with.

The key to this is that the underlying Session is tracked by an
Arc<Mutex<>>, with the related objects ensuring that they lock this
before they call into the underlying API.

In order to make this work, I've had to adjust the API around iterating
both known hosts and agent identities: previously these would iterate
over internal references but with this shift there isn't a reasonable
way to make that safe.  The strategy is instead to return a copy of the
host/identity data and then later look up the associated raw pointer
when needed.  The purist in me feels that the copy feels slightly
wasteful, but the realist justifies this with the observation that the
cardinality of both known hosts and identities is typically small enough
that the cost of this is in the noise compared to actually doing the
crypto+network ops.

I've removed a couple of error code related helpers from some of
the objects: those were really internal APIs and were redundant
with methods exported by the Error type anyway.

Fixes: #154
Refs: #137
Heh, I removed this by accident at the last minute before submitting
the PR to the CI... restore it!
This allows Channel and Stream to be Send
@wez
Copy link
Collaborator Author

wez commented Jan 19, 2020

@spebern I'd appreciate you taking a look over this when you have some cycles: it builds in part on the accessors you added in your recent PR, except that they lock. I think this will be fine for your async bindings: the locks are held only during the underlying API calls, and in non-blocking mode these should all be short lived.

@richardwhiuk requested this.  While it is safe to move to another
thread, it will still lock internally, and if you are in blocking
mode that may be undesirable.
src/sftp.rs Outdated Show resolved Hide resolved
and not the last sftp error.  Confusing!
@spebern
Copy link
Contributor

spebern commented Jan 23, 2020

Could Agent, Sftp and File be Send as well now?

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

Successfully merging this pull request may close these issues.

Session should not implement Send (at least if there is no mutual exclusion)
2 participants