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

Avoid concurrent requests for getting RSD service for a device #462

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nicolasbouffard
Copy link
Contributor

This PR prevents concurrent requests for getting RSD service for a device, based on the trio formed by address, port, and TUN port (even if it's zero). I did this because I noticed my program getting stuck multiple times in this function and ended up writing a debug program that calls the function multiple times asynchronously and got a freeze every single time, whereas running it synchronously or by preventing concurrent access gave 100% of success rate.

This might be related to #460 in the sense that this is a problem that may never happen if we are "lucky" and no concurrent accesses are made, but that probability increases dramatically when more requests are made in parallel. The issue is most likely not related to go-iOS but it might be the easiest place to "fix" it, at the cost of loosing some parallelization capability.

@danielpaulus danielpaulus requested a review from dmissmann August 19, 2024 21:48
@dmissmann
Copy link
Collaborator

I would rather move the responsibility of locking something to a place where no global state needs to be added. The only place I can think of where the concurrent access is happening is when we start a tunnel (getUntrustedTunnelServicePort) at the same time as connecting to a device for which we go a MDNS response (FindDeviceInterfaceAddress).
Did you see the issues when you were running go-ios tunnel start?

@nicolasbouffard
Copy link
Contributor Author

I think it will be easier if I show you : I cleaned up my debug program to make easy to share in this less than 400 lines of code repository.

Please feel free to read it, clone it, run it (as per the README.md's instructions), and ask questions if you still have any.

As per your suggestion, I'm open to the idea of moving the locking responsibility wherever it makes the most sense. It's just that it didn't seem to me that there already is a suitable entity to hold that responsibility. So between creating a completely new one and just putting a private global variable, I chose the least disturbing solution in my opinion 😇

Finally, I'd like to remind that I'm not sure at all of what I'm doing : maybe there's a way better idea to fix this issue, maybe it's even a bad idea to fix it like this for some reason, maybe I've just been extremely lucky in my runs and this isn't even a concurrency problem (that one would be crazy though 😅), etc. In the end I just wanted to share this issue with the community, alongside what looks to me like a potential solution after quick testing.

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.

2 participants