Replace keydown with hidden input #4
Conversation
|
I've also added the try catch i've talked about in #5 |
deno/src/utils/BroadcastMethods.ts
Outdated
| chan.postMessage({ error, requestId }) | ||
| try{ | ||
| chan.postMessage({ error, requestId }) | ||
| }catch(e){} |
There was a problem hiding this comment.
Is there anyway to match the error for this specific situation as if it's only a development issue the server shouldn't silently fail in production. Also definitely include a console.error('BroadcastChannel is dead failed to send errror', error)
There was a problem hiding this comment.
Honestly i don't think it's dev only...i think the server silently restart in prod so it goes unnoticed. If you have a way to check your production logs we might be able to found out if i'm right. I'll add a console.error btw.
deno/src/utils/BroadcastMethods.ts
Outdated
| @@ -40,7 +40,12 @@ export class BroadcastMethods<T extends Record<string, Function>> { | |||
| const result = await methods[fn](...args) | |||
| chan.postMessage({ result, requestId }) | |||
| } catch (error) { | |||
| chan.postMessage({ error, requestId }) | |||
| console.error('Can\'t post results, trying to send the error', error) | |||
There was a problem hiding this comment.
What is the specific error? It would be bad if this always silently failed? Doesn't this mean this broadcast channel should now be abandoned and a new one needs to be created? Why exactly is this failing?
If you could do something like the below, filling in the {{TEMPLATED}} sections that would be very useful.
| console.error('Can\'t post results, trying to send the error', error) | |
| if (error?.message?.includes('{{SOME_SPECIFIC_ERROR}}')) { | |
| console.error('Non-fatal error detected {{WHY_THIS_ERROR_HAPPENED}}') | |
| } else { | |
| throw error | |
| } |
There was a problem hiding this comment.
Oh got it, I'll check and see what is the actual error.
There was a problem hiding this comment.
Ok the exception is because the websocket i'm trying to send a message to has already closed. Fumbling with this it seems that there are also other uncaught exceptions all regarding this kind of issues (if someone leaves and the game is still open it will still fail).
I'm still not entirely sure on the reproduction steps but given it's not the only point where the disconnection can cause unhandled exceptions i think it might be better to do a complete check in another issue (i can do it myself but i might need your help to better understand the backend code without the need to read the whole codebase).
Let me know how do you want to handle this, if you prefer i might add an error similar to what you've proposed in the meantime.
There was a problem hiding this comment.
Does it throw this error if you connect two tabs to the game and one tab closes ?
There was a problem hiding this comment.
It's kinda strange because that's exactly the other error i was talking about: if you enter with two person and quit with one it will throw an unhandled error but only at the end of the game not during the "typing events".
deno/src/userWebSockets.ts
Outdated
| @@ -19,7 +19,9 @@ export function addWebSocket(socket: WebSocket, user?: User) { | |||
| if (currentSocket !== socket) { | |||
| const send = <T extends WebSocketResponse>(data: T) => { | |||
| try { | |||
| socket.send(JSON.stringify(data)) | |||
| if(socket.readyState === socket.OPEN){ | |||
There was a problem hiding this comment.
@mfbx9da4 i've finally come around to take a look at the server code: what was happening was that it tried to send a message before broadcasting, but given that the socket was not open it would've failed to send and call the destroy method that would've also close the channel.
I've added a check for the readystate of the socket before trying to send and this should fix the error that would cause the server to crash.
I was able to find a reproduction if you want to try on your own:
- open a tab and start a race
- open another tab and start a race
- wait for the race to start
- write at least a letter of the race
- close one tab
- refresh the other tab
- start the race again
This is what caused the server to crash before (it will not crash with this fix)
There was a problem hiding this comment.
tried to send a message before broadcasting
Isn't the issue that the user left the game and so the web socket connection is now closed and we don't gracefully handle connections being closed?
If the user's connection has dropped, it's fine to just not send messages. So I think your change makes a lot of sense.
There was a problem hiding this comment.
Isn't the issue that the user left the game and so the web socket connection is now closed and we don't gracefully handle connections being closed?
Yeah basically the websocket it's closed, in the broadcast method it calls the send method which tries to send a message to the websocket, failing to do so it closes the channel. After the send method the broadcast method tries to post a message but the channel was just closed by the websocket and so it fails.
There was a problem hiding this comment.
@mfbx9da4 any news? Do i need to do something else or can we merge this?
There was a problem hiding this comment.
Yeah just one final thing, could you run prettier on your changes? Also there should be a
// A space after comments ✅
//Not like this ❌
Thank you!
There was a problem hiding this comment.
All right...did it and pushed a new commit 😉
There was a problem hiding this comment.
It doesn't look like you ran prettier?
There was a problem hiding this comment.
Uh i don't know what happened...redid it
There was a problem hiding this comment.
@mfbx9da4 don't know if you missed this
|
Awesome thanks 🎉 |
I've tested this on PC, Chrome and it should work exactly the same.
I've tryed to keep the behavior just like the keydown event. This should let also mobile users use this website.
Let me know if that's ok.
Solves issue #3
EDIT: while testing the app the ws server was constantly crashing if I refreshed the tab before the timer run out. I don't know if this is specific to dev or it happens in production too.