Rapsssito / react-native-tcp-socket

React Native TCP socket API for Android, iOS & macOS with SSL/TLS support.
MIT License
315 stars 80 forks source link

Some events are not emitted in some Android devices #54

Closed mateo-gallardo closed 4 years ago

mateo-gallardo commented 4 years ago

Description

On some Android devices some events emitted via the sendEvent method in TcpSocketModule are not received in the JS side. For some reason it seems to be fixed with a Thread.sleep(100) right before any event is emitted in the TcpReceiverTask. Right now I have it before the instantiating the BufferedInputStream and it doesn't fail, but I don't understand why.

Thread.sleep(100);
BufferedInputStream in = new BufferedInputStream(socket.getInputStream());
while (!isCancelled() && !socket.isClosed()) {

Do you have any idea why this could be happening? I'm guessing it's giving time for another task in another thread to be ready for these events, but I don't know what it is.

Steps to reproduce

Steps to reproduce the behavior: It's hard to reproduce because it doesn't happen in every device. I've got a Pixel 3 where it works perfectly and a HUAWEI MediaPad 5 where it fails quite often.

I create a connection, send the data and close it immediately. The Pixel 3 failed 0 out of 100 events emitted. The HUAWEI tablet failed 51 out of 100 events emitted. The HUAWEI tablet with the Thread.sleep(100) failed 0 out of 100 events emitted.

Relevant information

| OS | Android | | react-native | 0.62.2 | | react-native-tcp-socket | 3.7.1 |

Rapsssito commented 4 years ago

@mateo-gallardo, thanks for your detailed explanation! Could you provide the code you used to test it?

mateo-gallardo commented 4 years ago

The server would concatenate all the JSON data received during the connection and parse it on close, that's how I know it failed, at no point the data event is fired and the parsing fails with Unexpected EOF.

let errorCount = 0;

TcpSocket.createServer(socket => {
  let json = '';

  socket.on('data', data => {
    console.log(`Received data ${data.toString()}`);
    json += data.toString();
  });

  socket.on('error', error => {
    console.log(`An error ocurred with client socket ${JSON.stringify(error)}`);
  });

  socket.on('close', () => {
    try {
      // Parsing on close since onData could be called several times with parts of the JSON for big JSONs
      const parsed = JSON.parse(json);
      console.log('Parsed successfully');
    } catch (error) {
      errorCount++;
      console.log(`Error parsing JSON ${error.message}. Error count: ${errorCount}`);
    }

    console.log('Closed connection');
  });
}).listen({
  port: 12345,
  host: '0.0.0.0'
}, serverInfo => {
  console.log(`TCP Socket open on ${serverInfo.address}:${serverInfo.port}`);
});

this.server.on('error', error => {
  console.log(`An error ocurred with the server ${JSON.stringify(error)}`);
});

this.server.on('close', () => {
  console.log('Server closed connection');
});

The client I'm using is actually a native iOS app, but any connection will do. Just a 0..100 for loop that sends the same JSON.

Rapsssito commented 4 years ago

@mateo-gallardo, thanks for the code! As I understand it, the issue is that the 'close' event is being called before the 'data' event, even though the socket sends the data before it closes the connection. Is that right?

mateo-gallardo commented 4 years ago

That was my suspicion at first, but logging the native Android code revealed that the events where being fired, just not received on the JS side. That Thread.sleep 'fixes' it for some reason, even with a 1ms sleep I got 1 error of out 300 tries.

I did quite a bit googling, but I couldn't find anything related to RN's events and background threads.

Rapsssito commented 4 years ago

@mateo-gallardo, I think it is related to RCTDeviceEventEmitter not being thread safe. I will get in touch as soon as I find something. Is this also happening on iOS?

mateo-gallardo commented 4 years ago

Great thanks! Let me know if you have any theories/code that I can test out. iOS works just fine 👍

mateo-gallardo commented 4 years ago

I tried running the sendEvent method in the main thread posting Runnables in Handler (new Handler(Looper.getMainLooper())) like they do in this PR but no luck.

I haven't tried queuing them up and sending them FIFO but I assume that running them in the same thread would guarantee that.

Rapsssito commented 4 years ago

@mateo-gallardo, could you try changing sendEvent() from private void to private synchronized void?

mateo-gallardo commented 4 years ago

Just tried it, sadly it didn't work.

Rapsssito commented 4 years ago

@mateo-gallardo, I cannot reproduce the issue by myself. Could you provide me with a minimal reproducible code that I could test against?

mateo-gallardo commented 4 years ago

The main factor with this issue is hardware, as I said in the Pixel 3 works fine but in the Huawei tablet it doesn't. I'll make a minimal app with the TCP server and a NodeJS client that sends a lot of JSONs and get back to you.

Rapsssito commented 4 years ago

@mateo-gallardo thanks so much! I also have a Pixel 3, but I know a couple friends with different Android phones. With your minimal code I would be able to get to the solution much faster.

mateo-gallardo commented 4 years ago

Ok, here's the app that will display how many errors parsing the JSON it gets. https://github.com/mateo-gallardo/tcp-socket-error-test-app

App image that shows an error count of 36

And here's the "client" that'll connect to the app and send a JSON every 1 second until you quit the process. Make sure you change the IP in the index.js file before running it. https://github.com/mateo-gallardo/tcp-socket-error-test-client

Screenshot of the nodejs console

It fails quite often in the Huawei tablet, but never fails in my Pixel 3.

Rapsssito commented 4 years ago

@mateo-gallardo, using your code changing the JSON timeout to 1 millisecond I am able to reproduce the Unexpected EOF exception in a Pixel 3. I am investigating.

Rapsssito commented 4 years ago

@mateo-gallardo, I think I found something. Could you try to swap the lines 32 and 33 in TcpSocketServer.java? It would look like this:

Socket socket = serverSocket.accept();
int clientId = getClientId();
TcpSocketClient socketClient = new TcpSocketClient(mReceiverListener, clientId, socket);
socketClients.put(clientId, socketClient);
// socketClient.startListening(); // Comment this line
mReceiverListener.onConnection(getId(), clientId, new InetSocketAddress(socket.getInetAddress(), socket.getPort()));
socketClient.startListening(); // Add this line
mateo-gallardo commented 4 years ago

YES! That fixed it, thanks! Works perfectly in the example app and the app I'm working on. Great job!

github-actions[bot] commented 4 years ago

:tada: This issue has been resolved in version 4.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: