Getting TCP Client Receives a Non-Integer

[dalabarge]

Getting TCP Client Receives a Non-Integer

I'm getting a significant in uptick of ANRs relating to getTcpClient(cid) where cid is not an integer:

java.lang.IllegalArgumentException: 
  at com.asterinet.react.tcpsocket.TcpSocketModule.getTcpClient (TcpSocketModule.java:251)
  at com.asterinet.react.tcpsocket.TcpSocketModule.access$700 (TcpSocketModule.java:28)
  at com.asterinet.react.tcpsocket.TcpSocketModule$2.run (TcpSocketModule.java:106)
  at java.util.concurrent.ThreadPoolExecutor.runWorker (ThreadPoolExecutor.java:1167)
  at java.util.concurrent.ThreadPoolExecutor$Worker.run (ThreadPoolExecutor.java:641)
  at java.lang.Thread.run (Thread.java:923)

Current behavior

It appears that the cid passed to getTcpClient() in TcpSocketModule.java when calling end() is not an integer. The JS side internally manages the _id and therefore at some point the _id is a non-integer triggering an exception when this package passes the _id to the end() or destroy() method as a cid. Seems to be occuring since upgrading from 4.x to 5.x

Expected behavior

Only way for that to happen is for the JS to lose a reference or reset the internal _id reference and pass it over the bridge. The bridge should protect against such errant calls as a noop to avoid the crash.

Relevant information

OS	Android 10-12
react-native	0.66.4
react-native-tcp-socket	5.5.0

[Rapsssito]

@dalabarge, could you provide a reproducible code? I am having issues reproducing your problem.

[dalabarge]

Not able to produce a repeatable trace. I believe it's caused by a Java exception because the internally tracked _id which is passed from the JS wrapper to the Java side as the cid has been unset. I believe this is because at some point in my code I'm calling socket.end() or .destroy() and it must be getting called twice through an async race condition. If the JS wrapper were to catch the second .end() call and do some safety check on it to see that _id is no longer set, it could either noop or throw a JS exception that my code can then catch. As it is now, it doesn't do any safety check so it goes straight to Java side and then throws an uncatchable error which crashes the app. Based on that, perhaps you can trace your code to see how _id would get unset and verify if my assumption is right. Then if it is, you could add the safety checks so that even if my code calls .end() too many times, I at least get a catchable error. I'm working my side to see if I can avoid the exception altogether.

[Rapsssito]

@dalabarge, I think it could be caused by calling end() on a not yet connected socket. Could this be the case?

[dalabarge]

My pseudo-code flow without all the app specific stuff is as follows. I've annotated with rationale and possible issue within my close() method. That said it appears socket.destroy() (or end()) are not idempotent when called.

class Component
{
    constructor() {
        this.socket = null
        this.close.bind(this)
    }
    
    connect() {
        // do a bit of cleanup to ensure clean state before creation new socket
        this.close()
        
        NetInfo.fetch('wifi')
            .then((net) => {
                // make sure we're using wifi only
                if( net.type !== 'wifi' ) return
                
                const socket = TcpSocket.createConnection(options), () => {
                    // set the active socket reference for use outside connect()
                    this.socket = socket

                    // apply a connection idleness timeout
                    if( Platform.OS === 'android' ) {
                        this.socket.setTimeout(10000, this.close)
                    }
                })

                socket.on('data', (data) => {
                    // ...

                    // inside a state manipulation I do call the
                    // socket.setTimeout() with a different duration
                    // but there's no direct calls to close() to reset socket
                })

                socket.on('close', () => {
                    // need to cleanup, especially if connection dropped without error
                    this.close()

                    // ...
                })

                socket.on('error', (error) => {
                    // an error should trigger a connection close so no need to cleanup here
                    // ...
                })
            })

            // anything goes wrong cleanup socket
            .catch(this.close)
    }

    close() {
        // noop indempotent guard
        if( ! _.isNil(this.socket) ) {
            try {
                // should be safe because socket should exist
                // ideally I should be able to call indempotently
                this.socket.destroy()
            } catch(error) {
                // noop handler because not interested in errors in cleanup
            }
            
            // Possibly this line should move above the try/catch to
            // ensure a race time duplicate call doesn't also
            // attempt to destroy the socket while inprogress already?
            this.socket = null
        }
    }
}

[Rapsssito]

@dalabarge, you are right, socket.destroy() and socket.end() are not idempotent. I must check NodeJS' net API to make sure theirs are and change ours socket.destroy() and socket.end() accordingly. This might fix your issue.

[github-actions]

🎉 This issue has been resolved in version 5.6.1 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀