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

Fix bridge client lifetime in spin context #35

Merged
merged 5 commits into from
Sep 11, 2022
Merged

Conversation

ssnover
Copy link
Collaborator

@ssnover ssnover commented Sep 4, 2022

I was looking to tackle the issue where the Client::stubborn_spin function would hold onto a Client forever and it turned into a rabbit hole. I ended up moving all of the members to a ClientInner struct which is co-owned through Arc<RwLock<T>> by the spin context and by all Clients.

In the nominal case, the lock is only being accessed as a reader because all of the members themselves are only used immutably (at least directly). This should mean that all accesses are very fast. However, if it disconnects from the bridge, the spin context takes a write lock which will be exclusive and allows it to overwrite the transport.

This is work in progress right now. One remaining task is there needs to be protection against starving of the write lock since it's totally possible for a user who relies on the stubborn reconnect to spam Client methods and for the write lock to be unobtainable.

…nce counts easier and make spin exit when all are dropped
@ssnover ssnover requested a review from Carter12s September 4, 2022 08:03
@ssnover ssnover linked an issue Sep 4, 2022 that may be closed by this pull request
@Carter12s
Copy link
Collaborator

The diff for this one is a mess, but in principle I like the changes. I might name things slightly differently like ClientHandle and Client vs. Client and ClientInner.

With how messed up the diff is pretty hard to tell what state this is really in, but let me know when its not WIP and I'll take a closer look.

roslibrust/src/rosbridge/publisher.rs Outdated Show resolved Hide resolved
roslibrust/src/rosbridge/mod.rs Outdated Show resolved Hide resolved
@ssnover
Copy link
Collaborator Author

ssnover commented Sep 5, 2022

Main remaining item I want to address is prevent soft deadlocks by users repeatedly retrying methods while waiting for the reconnect and the reconnect being starved of the write lock.

@Carter12s
Copy link
Collaborator

Main remaining item I want to address is prevent soft deadlocks by users repeatedly retrying methods while waiting for the reconnect and the reconnect being starved of the write lock.

This is an awesome thing to have an integration test around. I think beyond features, the most important thing we can do to make this crate have value is make it bulletproof so when we see edge cases that might exist in the API that rust can't directly guard against we should strive to cover those with tests.

@ssnover ssnover changed the title Work in progress: Fix bridge client lifetime in spin context Fix bridge client lifetime in spin context Sep 9, 2022
@ssnover
Copy link
Collaborator Author

ssnover commented Sep 9, 2022

This is no longer work in progress and should be ready for a full review!

@Carter12s
Copy link
Collaborator

Did you intend for this to Merge into master?

@ssnover
Copy link
Collaborator Author

ssnover commented Sep 9, 2022

Oh yeah, I targeted at the other branch because that branch was still in review. It looks like we need to close branches when they're merged since we have a lot of stale branches.

@ssnover ssnover changed the base branch from crate-reduction-with-features to master September 9, 2022 18:25
@ssnover
Copy link
Collaborator Author

ssnover commented Sep 9, 2022

The diff is much cleaner now actually. That's a little unexpected.

@Carter12s Carter12s merged commit 87bf794 into master Sep 11, 2022
@Carter12s Carter12s deleted the client-lifetime branch September 14, 2022 18:29
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.

Kill stubborn_spin on drop of client
2 participants