facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
118.13k stars 24.2k forks source link

Unexpected Websocket fail with big messages on Android #39651

Closed lauhon closed 2 weeks ago

lauhon commented 11 months ago

Description

I have a Websocket implementation that needs to send big chunks of Data at times.

The biggest chunk is a string with a length of 26858790 characters.

On iOS this is not a problem, but when running the same code on Android the Socket hangs up automatically with code 1001:

{"code": 1001, "isTrusted": false, "reason": ""}

This indicates to me, that there is some default setting in the Android implementation of Websockets. Sadly I don't have obvious access to that via the provided react-native Websocket API.

Please help! :)

React Native Version

0.72.5

Output of npx react-native info

System:
  OS: macOS 13.5.2
  CPU: (10) arm64 Apple M1 Pro
  Memory: 70.63 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 18.18.0
    path: ~/.volta/tools/image/node/18.18.0/bin/node
  Yarn:
    version: 1.22.19
    path: ~/.volta/tools/image/yarn/1.22.19/bin/yarn
  npm:
    version: 9.8.1
    path: ~/.volta/tools/image/node/18.18.0/bin/npm
  Watchman:
    version: 2023.06.12.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.12.1
    path: /opt/homebrew/bin/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 22.2
      - iOS 16.2
      - macOS 13.1
      - tvOS 16.1
      - watchOS 9.1
  Android SDK:
    API Levels:
      - "29"
      - "30"
      - "33"
    Build Tools:
      - 30.0.3
      - 33.0.0
    System Images:
      - android-33 | Google APIs ARM 64 v8a
      - android-33 | Google Play ARM 64 v8a
    Android NDK: Not Found
IDEs:
  Android Studio: 2022.3 AI-223.8836.35.2231.10406996
  Xcode:
    version: 14.2/14C18
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 11.0.20
    path: /usr/bin/javac
  Ruby:
    version: 2.7.8
    path: /opt/homebrew/opt/ruby@2.7/bin/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react:
    installed: 18.2.0
    wanted: 18.2.0
  react-native:
    installed: 0.72.5
    wanted: 0.72.5
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: Not found
  newArchEnabled: Not found

Steps to reproduce

  1. Setup a react-native project
  2. Create Websocket
    const ws = new WebSocket(...)
  3. Somewhere add a snippet like this:

      let myMsg = '';
      while (newmsg.length < 26858780) {
        myMsg = myMsg + "a";
      }
    
    ws.send(JSON.stringify({type: "inProgress", message: myMsg});

This will produce the described error

Snack, screenshot, or link to a repository

https://github.com/lauhon/react-native-android-websocket-bug

github-actions[bot] commented 11 months ago
:warning: Newer Version of React Native is Available!
:information_source: You are on a supported minor version, but it looks like there's a newer patch available - 0.72.5. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.
lauhon commented 11 months ago

Some more background:

The server that I want to talk to is implemented via fastify with fastify-websocket

There I configured the server like this:

import websocketPlugin from '@fastify/websocket';

 server.register(websocketPlugin, {
    options: { perMessageDeflate: true, maxPayload: 100 * 1024 * 1024, clientTracking: true },
  });

Wanted to share this to get across that I have a bigger messagePayload than my error-producing message is long. :)

cortinico commented 11 months ago

This is a link to my repository and the branch that basically should work with android Superlight-Labs/Superlight@68-android-support-part-2

Coudl you use the reproducer template which is linked by the bot to reproduce this problem?

lauhon commented 11 months ago

Sorry, I was confused by the initial Issue template and thought the reproducer was somehow voluntary.

here is a minimal reproduction.

It has a small node server that can be setup like yarn install yarn dev

afterwards the app/client can be started with yarn android

I experimented a bit, it seems the threshold seems to be exactly 16MB + 1 As soon as you try to send 16MB + 2 the error occurs as described.

I hope the example works for you and everything is clear, thank you!

lauhon commented 11 months ago

Hey, @cortinico is there anything more I can do to support this Issue?

cortinico commented 11 months ago

Hey, @cortinico is there anything more I can do to support this Issue?

I would suggest to start investigating what's the culprit and potentially send a PR to fix it

lauhon commented 11 months ago

I can do that.

Could you point me to a relevant place in the react-native code, where I could start investigating?

This would be my first contribution and I have basically no idea how WebSockets are implemented in react-native android.

I found this directory but it does not indicate any difference between android or ios implementation.

lauhon commented 11 months ago

Nvm. after some digging i found this module

Will look into it and come back later

lauhon commented 11 months ago

Hey @cortinico, quick update:

okHttp, which is used for networking on android, has a hard cap for messages of 16MiB

I opened a PR on their repo which would make the max message size configurable.

How should we continue here on react native side if/after they merged it? Should people be able to configure this on JS side, or should there be a hard cap? I assume on iOS there is also some hard cap..

For reference: ws enables people to configure it and they have a default hard cap set to 100MiB

cortinico commented 11 months ago

How should we continue here on react native side if/after they merged it?

Let's wait for a response on OkHTTP's end and we can get back to it on our end 👍

swankjesse commented 11 months ago

I think this might end badly. OkHttp’s cap is intended to limit queueing on the web socket. If you need to send a large payload, you should do it on a regular HTTP post, and send control messages on the web socket?

As is this will disrupt the latency of the web socket while the oversized message is being transmitted.

lauhon commented 11 months ago

Hey @swankjesse,

so basically you are saying that making the max queue size configurable is dangerous for okhttp consumers, because it's a bad practice that might lead to bugs down the road, right?

Sending large websocket messages like this is bad practice without a question. I still have some counter-points, please consider them and maybe we can move forward with this (via closing or merging) :)

  1. For react-native consumers the difference between iOS and android here seems to be against react-natives whole selling point, which is to use the same code for the different platforms. Having this inconsistency breaks this. Which is exactly why I came up with the Issue in the first place. I tried something -> it worked on iOS -> I was suprised by the socket hangup on android (without a meaningful error message btw).

  2. I don't think a technology has to protect its users from bad practices at all costs. While the low hard cap prevents some bad scenarios for sure, maybe it is also preventing some fast innovation for someone who simply wants to create a proof of concept or something, where not all best practices need to be followed 100% of the time.

  3. The okhttp pr would still leave the 16MB cap as a default, it simply adds the possibility of overriding this default setting. The possibility of configuring the maximum queue/message also exists in other websocket libraries, so why not in okhttp?

swankjesse commented 11 months ago

In OkHttp we optimized our implementation for small low-latency messages.

In particular:

It’s my belief that web sockets work best with these optimizations. Our implementation is simpler & faster. Our API is also simpler.

swankjesse commented 11 months ago

Would you consider buffering outbound messages in ReactNative’s adapter? OkHttp’s web socket tells you how big its outbound queue is, and you can write business logic to promote messages from your queue into OkHttp’s when the outbound queue is sufficiently small.

The queue size isn’t currently observable, though I’d be happy to fix this.

Now that I think of it, we could even allow messages to exceed the size limit when there are no other messages enqueued. This would satisfy your use case (unlimited queue size and unlimited message size) without introducing complexity in OkHttp. You’d need some code to manage the queue, and even that shouldn’t be too much work.

(Your only exposure is avoiding exhausting memory when a web socket enqueues messages faster than they can be transmitted. How does iOS handle this? Hopefully not by crashing the app!)

lauhon commented 11 months ago

So basically what you are saying is that having the option to configure a larger queue is not responsible because the messages will be kept in memory for too long?

TBH this is a little bit over my head here. I'm not a react-native maintainer per se, just a random person who stumbled upon this. I cannot decide which way react-native should handle this (although I would be happy to contribute as soon as the path forward is clear)

@cortinico - maybe you can give some input on this? :)

lauhon commented 10 months ago

@cortinico - bump

Kindly asking you to leave feedback about this thread

lauhon commented 6 months ago

Hey @cortinico - Its been a while, wondering if you have an opinion about the thread above :)

As I said, I'm happy to contribute as soon as its clear which way to go here

react-native-bot commented 3 weeks ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot commented 3 weeks ago

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

react-native-bot commented 2 weeks ago

This issue was closed because it has been stalled for 7 days with no activity.