-
Notifications
You must be signed in to change notification settings - Fork 51
Fix crash in React Native: add optional chaining to stream?.close() #668
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
Conversation
add optional chaining to stream close method
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for bringing this issue to our attention. We'll do some further investigations and testing on our end as well.
@@ -310,7 +310,7 @@ export abstract class AbstractRemote { | |||
clearTimeout(keepAliveTimeout); | |||
keepAliveTimeout = setTimeout(() => { | |||
this.logger.error(`No data received on WebSocket in ${SOCKET_TIMEOUT_MS}ms, closing connection.`); | |||
stream.close(); | |||
stream?.close(); | |||
}, SOCKET_TIMEOUT_MS); | |||
}; | |||
resetTimeout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the issue correctly, I think the stream
variable has not been assigned by the time the first timeout happens - perhaps due to the initial connection taking longer than the timeout.
I think we might be able to avoid this by removing the resetTimeout
call here which starts the timeout process before connector.connect()
has been called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make sense. I've updated the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work here! After digging deeper into this issue, I realized that we should handle a timeout during the initial connect
call better. I've implemented that timeout functionality here #671
If that PR gets merged we can close this one. Thanks again for for bringing this issue to our attention and for all your efforts.
remove the resetTimeout call which starts the timeout process before connector.connect() has been called
@felixgourdeau Thanks again for your efforts. The work here has been adopted in #671. We'll release the fix soon. |
This PR fixes a fatal crash (TypeError: Cannot read property 'close' of undefined') occurring in React Native environments.
The issue appears when the app is in the background and a periodic background process triggers. Just before the crash, the last log message is:
[PowerSyncRemote] No data received on WebSocket in 30000ms, closing connection.
The root cause is that the stream object can be undefined at this point. Adding optional chaining to the stream.close() call prevents the crash. However, this may mask a deeper issue—ideally, we should investigate why stream is sometimes undefined here.