Repository navigation
Conversation
…S socket ids JS numbers every Socket and Server it creates from a counter starting at 0, while the native side numbers the sockets a server accepts from its own counter (iOS: 5000 + n, Android: (1 + serverId) * 1000 + n). Nothing keeps the two ranges apart, and Server._buildSocket consumes a JS id for every accepted connection, so a long-running app eventually creates a JS socket whose id is already used by a live accepted connection. When that happens, connect()/listen() on the reused id fails with "this client's socket is already connected" (iOS) or "connect() called twice with the same id." (Android). The error is emitted for that id, so both JS sockets receive it, and the Socket error handler destroys the live accepted connection. Accepted sockets now get negative ids (-1, -2, ...) from a single counter on each platform. JS ids are never negative, so the two ranges cannot overlap, and on Android two servers can no longer hand out overlapping ranges.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #246: a JS-created socket can reuse the id of a live accepted socket. The native
connect()/listen()then fails for that id, and the error destroys the live accepted connection.JS numbers sockets from 0 (
src/Globals.js), while natively accepted sockets are numbered5000 + non iOS and(1 + serverId) * 1000 + non Android. Nothing keeps the ranges apart, so once an app has created enough sockets in one session, they collide.Changes
Natively accepted sockets now get negative ids (
-1, -2, …) from a single counter per platform:TcpSockets.m):getNextIdreturns-(++_counter). TheCOUNTER_OFFSETdefine is removed.TcpSocketServer.java): replaced the per-server(1 + serverId) * 1000range with one sharedAtomicInteger, returning-incrementAndGet().JS ids start at 0 and only grow, so they can never be negative and the two id spaces can't overlap. On Android, two servers also stop sharing overlapping ranges. Using an
AtomicIntegerkeeps id assignment safe across the listen threads.Compatibility
Socket._id) and only compared for equality, never ordered or range-checked. I checkedsrc/,ios/andandroid/.GCDAsyncSocketread tag (long), which accepts negative values.Notes
this client's socket is already connectedafter about a day of uptime, at the moment the app opened a new client socket.Test plan
connectionevents still deliver working sockets (data, write, close) with negative idssecureConnection) still workCloses #246