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.

Sign up for GitHub

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

Jump to bottom

Replace keydown with hidden input #4

Merged
merged 6 commits into from
Oct 13, 2022
Merged

Replace keydown with hidden input #4

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7460f61
Replace keydown with hidden input
paoloricciuti Sep 27, 2022
78cc727
Fix server crash on second postMessage
paoloricciuti Sep 28, 2022
0eefb4c
Added console.errors in both catch
paoloricciuti Sep 28, 2022
258ce6a
Added check for readyState.OPEN before sending
paoloricciuti Oct 5, 2022
9fffce8
Run prettier and add space to comments
paoloricciuti Oct 12, 2022
ce6782c
Rerun prettier
paoloricciuti Oct 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion deno/src/userWebSockets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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){
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfbx9da4 any news? Do i need to do something else or can we merge this?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@paoloricciuti paoloricciuti Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right...did it and pushed a new commit 😉

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like you ran prettier?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh i don't know what happened...redid it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfbx9da4 don't know if you missed this

socket.send(JSON.stringify(data))
}
} catch (error) {
console.error('Failed to send to socket', data, error)
destroy()
Expand Down
7 changes: 6 additions & 1 deletion deno/src/utils/BroadcastMethods.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Owner

@mfbx9da4 mfbx9da4 Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh got it, I'll check and see what is the actual error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it throw this error if you connect two tabs to the game and one tab closes ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

try{
chan.postMessage({ error, requestId })
}catch(e){
console.error('BroadcastChannel is dead failed to send error', e)
}
}
}

Expand Down
52 changes: 45 additions & 7 deletions sveltekit/src/routes/race.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@
? 'var(--yellow)'
: 'var(--pink)'

$: {
// when the phase swap to playing focus the hidden input
if (phase === 'playing') {
hiddenInput.focus()
}
}

onMount(() => {
const currentUrl = new URL(browser ? location.href : 'https://example.com')
const isProduction = currentUrl.protocol === 'https:' ? true : false
Expand Down Expand Up @@ -159,14 +166,24 @@
latency = Date.now() - start
}

function handleKeyDown(e: KeyboardEvent) {
let hiddenInput: HTMLInputElement
let hiddenInputValue = ''
let previousHiddenInputValue = ''

function handleHiddenInput(e: Event) {
// keep track of previous hidden input and current hidden input
// to calculate the delta between the two
previousHiddenInputValue = hiddenInputValue
hiddenInputValue = (e.target as HTMLInputElement).value
if (phase !== 'playing') return
if (!e.metaKey) {
e.preventDefault()
}
// if the previous length is less than the current length return
// to keep the same behavior of the keydown event previously implemented
if (hiddenInputValue.length < previousHiddenInputValue.length) return
// replace all the previous value from the current value to get the new key
const key = hiddenInputValue.replace(previousHiddenInputValue, '')
const expecting = codeSnippetContent[progress]
if (expecting) {
if (e.key === expecting) {
if (key === expecting) {
progress++
if (codeSnippetContent[progress] === '\n') {
while (/\s/.test(codeSnippetContent[progress])) {
Expand All @@ -189,12 +206,27 @@
const colors = ['var(--pink)', 'var(--yellow)', 'var(--blue)', 'var(--green)', 'var(--purple)']
</script>

<svelte:window on:keydown={handleKeyDown} />
<svelte:window
on:focus={() => {
// on window focus focus the input (to avoid losing focus)
hiddenInput.focus()
}}
/>

<svelte:head>
<title>Coderacer</title>
</svelte:head>

<!-- preventing pasting into the input and blurring -->
<input
bind:this={hiddenInput}
type="text"
on:input={handleHiddenInput}
on:paste|preventDefault|stopPropagation
on:blur={() => {
hiddenInput.focus()
}}
class="hidden-input"
/>
<section style="padding: 20px">
<div style="margin: 0 auto; text-align: center; height: 20px">
{phase === 'won'
Expand Down Expand Up @@ -324,4 +356,10 @@
</div>

<style>
.hidden-input {
opacity: 0;
font-size: 16px;
position: absolute;
pointer-events: none;
}
</style>