handle exception when websocket server start room failed #289
Conversation
|
|
There was a problem hiding this comment.
Thanks @jzhang20133, could you please add a test that checks this logic?
| self.log.info("Deleting file %s", file.path) | ||
| await self._file_loaders.remove(file_id) | ||
| self._emit(LogLevel.INFO, "clean", "Loader deleted.") | ||
| raise e |
There was a problem hiding this comment.
Should the exception be raised, or just logged?
There was a problem hiding this comment.
it should be raised when task group is not active and the request to set up websocket will fail and UI will retry. We should not fail silently here if task group is not active.
jzhang20133
force-pushed
the
main-2
branch
4 times, most recently
from
a214112
to
f1bd86f
Compare
|
@davidbrochart I have added a unit test to make sure two collaborative warning event is not thrown and room and file is deleted when both websocket requests fails due to webserver task group is no longer active. |
jzhang20133
added
enhancement
|
Awesome work, @jzhang20133! It looks like PR also fixes #291 by moving the |
|
@jzhang20133 thanks for this! Just a minor comment about the Maybe a simpler approach is to call |
jzhang20133
force-pushed
the
main-2
branch
2 times, most recently
from
a1f24b4
to
401599b
Compare
|
Addressed comments. @davidbrochart and @Zsailer would you like to review this PR again? |
jzhang20133
force-pushed
the
main-2
branch
3 times, most recently
from
33b0632
to
a226c80
Compare
jzhang20133
force-pushed
the
main-2
branch
3 times, most recently
from
e6332b6
to
be477fb
Compare
|
Looks great! Thanks @jzhang20133! |
| self._websocket_server.add_room(self._room_id, self.room) | ||
| try: | ||
| await self._websocket_server.start_room(self.room) | ||
| except Exception as e: |
There was a problem hiding this comment.
Maybe I'm missing something, but didn't we introduced the YRoom exception handler just for that purpose? It seems we are bypassing all the work we did in pycrdt-websocket and instead directly handling the exception here.
| try: | ||
| await super().stop() | ||
| except RuntimeError: | ||
| pass |
There was a problem hiding this comment.
This should be done in pycrdt-websocket using the exception handler.
| try: | ||
| await super().stop() | ||
| except RuntimeError: | ||
| pass |
There was a problem hiding this comment.
This should be done in pycrdt-websocket using the exception handler.
(#298) Co-authored-by: Jialin Zhang <jialin_zhang4@apple.com>
In this PR, we handle exception when websocket server start room failed by cleaning up fileLoader and room and user won't run into the two collaboration pop up due to websocket server task group inactive failure.
This PR also move authentication logic ahead of room initialization.
Resolving:
#291
#245