Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

ThaliCore: maxVirtualSocketsCount bug #1893

Closed
enricogior opened this issue Jun 1, 2017 · 4 comments
Closed

ThaliCore: maxVirtualSocketsCount bug #1893

enricogior opened this issue Jun 1, 2017 · 4 comments
Assignees

Comments

@enricogior
Copy link
Member

enricogior commented Jun 1, 2017

maxVirtualSocketsCount is used to limit the max number of concurrent virtual sockets in use by the BrowserRelay but the check [here] can't work properly if an app opens multiple connections in a row. Given the async nature of the createVirtualSocket, the actual virtualSockets.value.count will only be increased later on when the completion handler is invoked.
I've verified it opening 30 connections without reaching the limit since all 30 calls to createVirtualSocket were completed before the completion handler is invoked.

@enricogior enricogior self-assigned this Jun 1, 2017
@enricogior
Copy link
Member Author

Even if the goal is to limit the max numbers of VirtualSockets, the check should be done at a higher level in the TCPListener class, before even trying to create a new VirtualSocket.

@enricogior
Copy link
Member Author

The max number of overall connections in the native layer is limited by the number of concurrent virtual sockets: each virtual socket adds an entry to RunLoop and the max number of entries per process is 64, that is enforced by the OS.
Adding more than 64 entries doesn't trigger an error in RunLoop, simply the entries after the 64th will not be scheduled until other entries have been removed.
That is one of the two bugs that were causing the timeout failure since the RunLoop entries were never removed.

The current code that limits each BrowserRelay to 16 connections, can't prevent to reach the overall number of virtual sockets created by the BrowseRelays and the AdvertiserRelays.
We should decide the max number of overall concurrent virtual sockets (i.e. it should be less the 64 since we have already at least one entry in the RunLoop for other purpose, so it should be no more than 62 to be safe).
I still have to decide where to put the check for the max overall virtual sockets and how to handle the error, since it's not a connection error per se, but just an internal limit.

We should also decide if we need to limit the max number of virtual sockets per BrowserRelay or per Advertiser Relay. It's not strictly needed but if there is a good reason to do so we should fix the current implementation since it's broken (see the first comment).

@yaronyg @mlesnic is there any reason to limit the max connections in each BrowserRelay or in each AdvertiserRelay? If so, what should the max be, 16?

@yaronyg
Copy link
Member

yaronyg commented Jul 10, 2017

So the point of this was really more than anything to try and slow down DOS attacks by people connecting to the device (e.g. on the advertiser, not the browser). But we never really did a good job here. We need #849 and most likely some kind of native event to let us know when we have an incoming connection. I'm o.k. with us either removing the limit and putting in a big warning in the docs that on Mac we can't have more than 64 peer connections in both directions. But we should file a bug saying we suck and we should fix this so we limit things ourselves and return proper errors.

enricogior added a commit to thaliproject/thali-ios that referenced this issue Jul 11, 2017
@enricogior
Copy link
Member Author

#1933
#1934

I've also removed the current (broken) limit in BrowserRelay.

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

No branches or pull requests

2 participants