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

Cannot read properties of undefined (reading 'id') #6050

Open
juliushaertl opened this issue Jul 15, 2024 · 8 comments
Open

Cannot read properties of undefined (reading 'id') #6050

juliushaertl opened this issue Jul 15, 2024 · 8 comments
Assignees
Labels
1. to develop bug Something isn't working

Comments

@juliushaertl
Copy link
Member

Sentry reported a quite frequent console error, that only seems there since "29.0.4 RC1"

Screenshot 2024-07-15 at 09 29 55

https://nextcloud-gmbh.sentry.io/share/issue/0d2e881a7a144773a4460df9cb99acd1/

Screenshot 2024-07-15 at 09 29 02
@max-nextcloud
Copy link
Collaborator

This is odd. I have not been able to reproduce it locally nor on the instance in question yet.
I wonder if this is related to our cleanup of the old sessions. But missing sessions should result in a 403 and then the message to the user to please reload the page.

@max-nextcloud
Copy link
Collaborator

I tried to reproduce this in various ways:

  • Editing with two users in the 29.0.4 version locally.
  • On cloud.nextcloud.com trying various things.
  • Setting up 29.0.2 locally and migrating to 29.0.4

I still failed to reproduce it. What I did run into was a situation where sessions was empty because during the update Nextcloud responded with a 200 maintenance page on push and sync. Even though this caused exceptions in the same function it does not seem related.

@max-nextcloud
Copy link
Collaborator

Looking at the code this is probably caused by the list of sessions not including any with the current id.
In this case sessions.find will return undefined and this.currentSession is set to undefined.

I see two possible causes:

  1. The server responds to a sync request with a list of sessions that does not include a session with the same id as the one that was used to authenticate the request.
  2. The currentSession id changed in the meantime. Maybe this even is a response to an old request that gets handled after the document changed or so. (Which might point to collectives as there one can change pages fairly quickly).

@juliushaertl
Copy link
Member Author

URLs seem mostly from files, collectives is just a low amount, top most is a read only share (from the app tutorials), but could just be due to the amout of people browsing it.

@juliushaertl
Copy link
Member Author

juliushaertl commented Jul 16, 2024

Interestingly browsers are just chromium based ones

@max-nextcloud
Copy link
Collaborator

max-nextcloud commented Jul 16, 2024

uuuhhh... we have this routine to clean up old sessions during the request. That happens after auth. So maybe one can still authenticate with a session and then have it cleaned up.

update: but we only do this during create and close. Create does not require any auth anyway - so maybe this happens when automatically reconnecting and the old session is still around somehow. Since the last update we don't clear the sync service anymore if i remember correctly.

@max-nextcloud
Copy link
Collaborator

I was able to trigger an inconsistent state by switching the network off and on again. After a while yjs will attempt to reopen the websocket, calling SyncService.open which fails to open a new connection but still continues. Afterwards sync services _receiveSteps fails as this.#connection is undefined.

grafik

This happens a few times until the editor shows the reconnect button. I suspect it's basically the polling backend continuing in the backend.

@juliushaertl
Copy link
Member Author

I also couldn't trigger the issue but I noticed when opening a read only link and keeping the browser running for a while we sometimes have additional create requests without a previous close

max-nextcloud added a commit that referenced this issue Jul 16, 2024
Do not attempt to create a new connection
if there already is one and it is not closed.

If no messages are received for 30 seconds
yjs will open a new websocket.

Since we do not close the connection anymore from the websocket polyfill
we also do not need to open it.

If the network connection has gone down
creating a new connection will fail anyway.

Once it comes back we will know if the session is still valid.
Then we can either continue using it or reconnect.

This is part of #6050.

Signed-off-by: Max <max@nextcloud.com>
max-nextcloud added a commit that referenced this issue Jul 18, 2024
Do not attempt to create a new connection
if there already is one and it is not closed.

If no messages are received for 30 seconds
yjs will open a new websocket.

Since we do not close the connection anymore from the websocket polyfill
we also do not need to open it.

If the network connection has gone down
creating a new connection will fail anyway.

Once it comes back we will know if the session is still valid.
Then we can either continue using it or reconnect.

This is part of #6050.

Signed-off-by: Max <max@nextcloud.com>
backportbot bot pushed a commit that referenced this issue Jul 18, 2024
Do not attempt to create a new connection

if there already is one and it is not closed.

If no messages are received for 30 seconds

yjs will open a new websocket.

Since we do not close the connection anymore from the websocket polyfill

we also do not need to open it.

If the network connection has gone down

creating a new connection will fail anyway.

Once it comes back we will know if the session is still valid.

Then we can either continue using it or reconnect.

This is part of #6050.

Signed-off-by: Max <max@nextcloud.com>

[skip ci]
backportbot bot pushed a commit that referenced this issue Jul 18, 2024
Do not attempt to create a new connection
if there already is one and it is not closed.

If no messages are received for 30 seconds
yjs will open a new websocket.

Since we do not close the connection anymore from the websocket polyfill
we also do not need to open it.

If the network connection has gone down
creating a new connection will fail anyway.

Once it comes back we will know if the session is still valid.
Then we can either continue using it or reconnect.

This is part of #6050.

Signed-off-by: Max <max@nextcloud.com>
max-nextcloud added a commit that referenced this issue Jul 19, 2024
Do not attempt to create a new connection
if there already is one and it is not closed.

If no messages are received for 30 seconds
yjs will open a new websocket.

Since we do not close the connection anymore from the websocket polyfill
we also do not need to open it.

If the network connection has gone down
creating a new connection will fail anyway.

Once it comes back we will know if the session is still valid.
Then we can either continue using it or reconnect.

This is part of #6050.

Signed-off-by: Max <max@nextcloud.com>
mejo- pushed a commit that referenced this issue Jul 24, 2024
Do not attempt to create a new connection
if there already is one and it is not closed.

If no messages are received for 30 seconds
yjs will open a new websocket.

Since we do not close the connection anymore from the websocket polyfill
we also do not need to open it.

If the network connection has gone down
creating a new connection will fail anyway.

Once it comes back we will know if the session is still valid.
Then we can either continue using it or reconnect.

This is part of #6050.

Signed-off-by: Max <max@nextcloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. to develop bug Something isn't working
Projects
Status: 🏗️ In progress
Development

No branches or pull requests

2 participants