Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Websocket Wrong Control Frame [iOS] #30020

Closed
vitalyrotari opened this issue Sep 24, 2020 · 15 comments
Closed

Websocket Wrong Control Frame [iOS] #30020

vitalyrotari opened this issue Sep 24, 2020 · 15 comments
Labels
Needs: Triage 🔍 Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.

Comments

@vitalyrotari
Copy link

There was already reported and it's mark as resolved, see: #23825. But in fact this problem is still persist on iOS. After many investigation the problem comes from iOS RN WebSocket library.

Description

Server disconnect client on control packets, so if ping is disabled from server, problem disappear.

Error from server based on NodeJS

Invalid WebSocket frame: invalid payload length 126

This error appear when server send data and in parallel send control packets to client.

React Native version:

0.62.2

@endel
Copy link

endel commented Sep 24, 2020

This looks like the PONG frame sent by react-native is causing this problem.

"All control frames must have a payload length of 125 bytes or less"
https://tools.ietf.org/html/rfc6455#page-36

In this scenario described by @vitalyrotari, the server-side is crashing due to what seems to be an invalid control frame being sent by the client.

@stale
Copy link

stale bot commented Dec 25, 2020

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as a "Discussion" or add it to the "Backlog" and I will leave it open. Thank you for your contributions.

@stale stale bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 25, 2020
@moneymc
Copy link

moneymc commented Jan 19, 2021

Same issue.

@stale stale bot removed the Stale There has been a lack of activity on this issue and it may be closed soon. label Jan 19, 2021
@vitalyrotari
Copy link
Author

any updates for this?

@MrXyfir
Copy link

MrXyfir commented May 9, 2021

This bug has been around since 2015... how has it never become a priority?

@knro
Copy link

knro commented Sep 11, 2021

Can confirm this issue still. Any workarounds? Is there a way to configure NodeJS server to catch this and gracefully handle it instead of crashing?

@MrXyfir
Copy link

MrXyfir commented Sep 11, 2021

@knro Depends on your server. If you're using ws on Node:

import WebSocket from 'ws';
...
const server = new WebSocket.Server({ ... });
server.on('connection', (socket, req) => {
  ...
  // Make sure to add this error handler here so your server doesn't crash
  // If you want, filter out the 126 error
  socket.on('error', (err) => console.error('socket error', err));
  ...
})

I continue to log this error in our production server just for fun and to get a sense of the scale of the problem. It happens constantly all day long. If we couldn't prevent the crash our app would be unusable.

@serjek
Copy link

serjek commented Sep 11, 2021

I just disabled built-in ping-pong and made my custom implementation.

@forbesgillikin
Copy link

I had not experienced this issue until recently after implementing graphql-ws

@serjek
Copy link

serjek commented Sep 18, 2021

I had not experienced this issue until recently after implementing graphql-ws

you mean you replaced default RN websocket lib with this one or what?

@asmeikal
Copy link
Contributor

@forbesgillikin we're using graphql-ws and we're having a similar problem, but it should be resolved in version 5.5.5 of graphql-ws: https://github.com/enisdenjo/graphql-ws/releases/tag/v5.5.5
The fix limits on JS side the max length of the close reason sent from the client.

For those using graphql-ws, the problem should not be related to ping/pong control frames, since the standard ping implementation of graphql-ws does not send any payload, and the react-native pong implementation just echoes it back (see here).
The ping from react-native seems to send always an empty payload too (see here).
Since with graphql-ws the problem seems to be caused by close frames with a reason that's too long, it shouldn't be really an issue, just an annoying error: the client already closed the connection, and a typical server closes it too when such an error happens.

The fix on the react-native side should be to do a check similar to this one when sending a frame in _sendFrameWithOpcode, and either truncate the message or report an error. The ws node library, for example, chooses to throw an error in this cases (see here).

@asmeikal
Copy link
Contributor

asmeikal commented Jan 6, 2022

Upgrading graphql-ws to 5.5.5 did not solve the problem for us: we're still seeing several warnings, and the actual bug seems to be with pong frames, not close frames.

What happens is:

  • server sends a ping frame
  • server sends a text frame
  • client responds to ping frame with a pong frame containing the payload of the text frame

This does not happen consistently, and may be a race condition on the RN implementation of websockets. Haven't yet had the time to look into it.

@asmeikal
Copy link
Contributor

asmeikal commented Jan 6, 2022

This bug seems to have been fixed in this PR and optimized this PR in the original websocket implementation. The bug is caused by concurrent mutation of the _currentFrameData, which in the react-native copy of the original implementation should happen here. The fix should be as quick as copying the frame data like so inside _handleFrameWithData.

@pastean
Copy link

pastean commented Jun 12, 2022

is this part of any react-native releases so far?

@cortinico
Copy link
Contributor

is this part of any react-native releases so far?

It was released on 0.68

@facebook facebook locked as resolved and limited conversation to collaborators Jan 20, 2023
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jan 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs: Triage 🔍 Platform: iOS iOS applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

Successfully merging a pull request may close this issue.