Fix: player session never recovers when server resets - #18
Conversation
… connecting fails 3 times
2c598a9 to
7a8c616
Compare
a149dc3 to
f72fbea
Compare
jbriones1
left a comment
jbriones1
left a comment
There was a problem hiding this comment.
The test comments can probably be ignored, but when tests are sus I'm sus of the code.
There was a problem hiding this comment.
The tests are kinda brittle, because they use hardcoded number for the delays, and the delay between retries isn't documented. This means if the delay was changed or was changed to an exponential backoff, then it would cause the tests to fail. I feel like they could be moved into a const array, similar to the websocket service.
But units tests amirite?
| it('does not invoke the session-expired callback after fewer than three failures', () => { | ||
| vi.useFakeTimers(); | ||
| vi.spyOn(console, 'log').mockImplementation(() => undefined); | ||
| vi.spyOn(console, 'warn').mockImplementation(() => undefined); | ||
| const onSessionExpired = vi.fn(); | ||
| service.start('ABCD', () => undefined, onSessionExpired); | ||
|
|
||
| MockGameWebSocket.instances[0].serverClose(false); | ||
| vi.advanceTimersByTime(1000); | ||
| MockGameWebSocket.instances[1].serverClose(false); | ||
| vi.advanceTimersByTime(2000); | ||
|
|
||
| expect(service.sessionExpired()).toBe(false); | ||
| expect(onSessionExpired).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('keeps reconnecting a player socket after fewer than three failures', () => { | ||
| vi.useFakeTimers(); | ||
| vi.spyOn(console, 'log').mockImplementation(() => undefined); | ||
| vi.spyOn(console, 'warn').mockImplementation(() => undefined); | ||
| service.start('ABCD', () => undefined); | ||
|
|
||
| MockGameWebSocket.instances[0].serverClose(false); | ||
| vi.advanceTimersByTime(1000); | ||
| MockGameWebSocket.instances[1].serverClose(false); | ||
| vi.advanceTimersByTime(2000); | ||
|
|
||
| expect(service.sessionExpired()).toBe(false); | ||
| expect(service.state()).toBe('connecting'); | ||
| expect(MockGameWebSocket.instances).toHaveLength(3); | ||
| }); |
There was a problem hiding this comment.
I feel like these tests can merged into one.
| }); | ||
|
|
||
| it('re-registers with the saved name and reconnects', async () => { | ||
| playerName.get.mockReturnValue('Odin'); |
|
|
||
| async function flushReRegistration(): Promise<void> { | ||
| fixture.detectChanges(); | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); |
There was a problem hiding this comment.
Same here. I guess it's because the mocks use of(...)? In that case these are better :
await vi.waitFor(() => {
expect(...).toHaveBeenCalledWith(...);
})
on tests that use render and flushReRegistration are probably better.
| async function render(): Promise<HTMLElement> { | ||
| fixture = TestBed.createComponent(GamePageComponent); | ||
| fixture.detectChanges(); | ||
| await new Promise((resolve) => setTimeout(resolve, 0)); |
There was a problem hiding this comment.
setTimeout(..., 0) calls are always kinda sus.
| return false; | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Remove the trailing whitespace
|
|
||
| const startCalls = gameSocket.start.mock.calls; | ||
| const onConnected = startCalls[startCalls.length - 1][1] as () => void; | ||
| expect(startCalls[startCalls.length - 1][0]).toBe('NEWID'); |
There was a problem hiding this comment.
nit: this doesn't check the third argument of the start function. In the future, if someone refactors and messes up and doesn't add a third parameter on reconnect this test would fail.
|
|
||
| protected override shouldReconnect(closeEvent: CloseEvent): boolean { | ||
| if (this.mode !== 'player') { | ||
| return true; |
There was a problem hiding this comment.
I don't think players should reconnect if the closeEvent was graceful (status 1001). I don't think our server is capable of having a good shutdown, so maybe this could be added.
With this logic, if server is shut down and someone never closed their browser tab, they would reconnect if the server was turned back and they opened their tab. Would be kinda weird.
|
|
||
| it.each(['http:', 'https:'] as const)('expires the id cookie over %s', (protocol) => { | ||
| mockWindow.location.protocol = protocol; | ||
| mockDocument.cookie = 'id=ABC; theme=dark'; |
There was a problem hiding this comment.
We don't support themes in our app.
There was a problem hiding this comment.
I feel like this could be merged into the credentials service.
|
Will take a look at how to get the server to explicitly send signals instead of frontend guessing when to reconnect so we're not retrying connection even on graceful server shutdowns! |
2 participants
Closes #12
Description:
When game restarts, player session cookies get invalidated so websocket can never reconnect so the player is always stuck on "Reconnecting..." forever. This change allows users to automatically re-register the player with a fresh session.
After 3 consecutie failed ocnnection attemptions, the client stops retrying and automatically re-registers the player with a fresh session with the saved player's name.
If the server is down, it'll lead the user back to the registration page.