-
Notifications
You must be signed in to change notification settings - Fork 0
Fix: player session never recovers when server resets #18
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
base: main
Are you sure you want to change the base?
Changes from all commits
1285d74
7a8c616
fe5a036
9e60e14
20cc73b
f72fbea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { TestBed } from '@angular/core/testing'; | ||
|
|
||
| import { PAC_WINDOW } from './browser-window.token'; | ||
| import { PlayerNameService } from './player-name.service'; | ||
|
|
||
| describe('PlayerNameService', () => { | ||
| let service: PlayerNameService; | ||
| let mockStorage: Record<string, string>; | ||
|
|
||
| beforeEach(() => { | ||
| mockStorage = {}; | ||
| const mockWindow = { | ||
| localStorage: { | ||
| getItem: (key: string) => mockStorage[key] ?? null, | ||
| setItem: (key: string, value: string) => { | ||
| mockStorage[key] = value; | ||
| }, | ||
| }, | ||
| }; | ||
|
|
||
| TestBed.configureTestingModule({ | ||
| providers: [PlayerNameService, { provide: PAC_WINDOW, useValue: mockWindow }], | ||
| }); | ||
| service = TestBed.inject(PlayerNameService); | ||
| }); | ||
|
|
||
| it('returns an empty string when no name has been saved', () => { | ||
| expect(service.get()).toBe(''); | ||
| }); | ||
|
|
||
| it('saves and retrieves a player name', () => { | ||
| service.save('Odin'); | ||
| expect(service.get()).toBe('Odin'); | ||
| }); | ||
|
|
||
| it('returns an empty string when localStorage throws on get', () => { | ||
| TestBed.resetTestingModule(); | ||
| const throwingWindow = { | ||
| localStorage: { | ||
| getItem: () => { | ||
| throw new Error('unavailable'); | ||
| }, | ||
| setItem: () => {}, | ||
| }, | ||
| }; | ||
| TestBed.configureTestingModule({ | ||
| providers: [PlayerNameService, { provide: PAC_WINDOW, useValue: throwingWindow }], | ||
| }); | ||
| service = TestBed.inject(PlayerNameService); | ||
| expect(service.get()).toBe(''); | ||
| }); | ||
|
|
||
| it('silently ignores localStorage errors on save', () => { | ||
| TestBed.resetTestingModule(); | ||
| const throwingWindow = { | ||
| localStorage: { | ||
| getItem: () => null, | ||
| setItem: () => { | ||
| throw new Error('quota exceeded'); | ||
| }, | ||
| }, | ||
| }; | ||
| TestBed.configureTestingModule({ | ||
| providers: [PlayerNameService, { provide: PAC_WINDOW, useValue: throwingWindow }], | ||
| }); | ||
| service = TestBed.inject(PlayerNameService); | ||
| expect(() => service.save('Odin')).not.toThrow(); | ||
| }); | ||
|
|
||
| it('returns an empty string when PAC_WINDOW is null', () => { | ||
| TestBed.resetTestingModule(); | ||
| TestBed.configureTestingModule({ | ||
| providers: [PlayerNameService, { provide: PAC_WINDOW, useValue: null }], | ||
| }); | ||
| service = TestBed.inject(PlayerNameService); | ||
| expect(service.get()).toBe(''); | ||
| expect(() => service.save('Odin')).not.toThrow(); | ||
| }); | ||
| }); |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like this could be merged into the credentials service. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { inject, Service, signal } from '@angular/core'; | ||
| import { PAC_WINDOW } from './browser-window.token'; | ||
|
|
||
| const PLAYER_NAME_KEY = 'playerName'; | ||
|
|
||
| @Service() | ||
| export class PlayerNameService { | ||
| private readonly browserWindow = inject(PAC_WINDOW); | ||
| private readonly statusMessage = signal<string | null>(null); | ||
|
|
||
| get(): string { | ||
| try { | ||
| return this.browserWindow?.localStorage.getItem(PLAYER_NAME_KEY) ?? ''; | ||
| } catch { | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| save(name: string): void { | ||
| try { | ||
| this.browserWindow?.localStorage.setItem(PLAYER_NAME_KEY, name); | ||
| } catch { | ||
| this.statusMessage.set('Could not save your name for auto-re-registration.'); | ||
| } | ||
| } | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -297,4 +297,118 @@ describe('GameSocketService', () => { | |
| expect(service.state()).toBe('error'); | ||
| expect(service.status()).toContain('Register as admin again in this browser'); | ||
| }); | ||
|
|
||
| it('expires the session after three consecutive failed player connections', () => { | ||
| 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); | ||
| MockGameWebSocket.instances[2].serverClose(false); | ||
| vi.runAllTimers(); | ||
|
|
||
| expect(service.sessionExpired()).toBe(true); | ||
| expect(service.state()).toBe('error'); | ||
| expect(service.status()).toContain('Session has expired as game server restarted.'); | ||
| expect(MockGameWebSocket.instances).toHaveLength(3); | ||
| expect(onSessionExpired).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| 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); | ||
| }); | ||
|
Comment on lines
+322
to
+352
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like these tests can merged into one. |
||
|
|
||
| it('resets the failure counter when a player connection succeeds', () => { | ||
| 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); | ||
|
|
||
| MockGameWebSocket.instances[2].open(); | ||
| MockGameWebSocket.instances[2].serverClose(false); | ||
| vi.advanceTimersByTime(4000); | ||
| MockGameWebSocket.instances[3].serverClose(false); | ||
| vi.runAllTimers(); | ||
|
|
||
| expect(service.sessionExpired()).toBe(false); | ||
| expect(MockGameWebSocket.instances).toHaveLength(5); | ||
| }); | ||
|
|
||
| it('never expires the session for a viewer socket', () => { | ||
| vi.useFakeTimers(); | ||
| vi.spyOn(console, 'log').mockImplementation(() => undefined); | ||
| vi.spyOn(console, 'warn').mockImplementation(() => undefined); | ||
| service.startViewer(); | ||
|
|
||
| MockGameWebSocket.instances[0].serverClose(false); | ||
| vi.advanceTimersByTime(1000); | ||
| MockGameWebSocket.instances[1].serverClose(false); | ||
| vi.advanceTimersByTime(2000); | ||
| MockGameWebSocket.instances[2].serverClose(false); | ||
| vi.runAllTimers(); | ||
|
|
||
| expect(service.sessionExpired()).toBe(false); | ||
| expect(MockGameWebSocket.instances.length).toBeGreaterThan(3); | ||
| }); | ||
|
|
||
| it('start and stop clear an expired session', () => { | ||
| 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); | ||
| MockGameWebSocket.instances[2].serverClose(false); | ||
| vi.runAllTimers(); | ||
|
|
||
| expect(service.sessionExpired()).toBe(true); | ||
|
|
||
| service.stop(); | ||
| expect(service.sessionExpired()).toBe(false); | ||
|
|
||
| service.start('ABCD', () => undefined); | ||
| expect(service.sessionExpired()).toBe(false); | ||
| expect(MockGameWebSocket.instances).toHaveLength(4); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,18 +29,25 @@ export class GameSocketService extends WebSocketService<GameSocketMessage> { | |
| private playerId: string | null = null; | ||
| private mode: SocketMode | null = null; | ||
| private onConnected: (() => void) | null = null; | ||
| private onSessionExpired: (() => void) | null = null; | ||
| private reconnecting = false; | ||
| private suspendedReason = 'Paused while the browser is offline.'; | ||
| private consecutiveFailures = 0; | ||
| private readonly statusMessage = signal<string | null>(null); | ||
|
|
||
| readonly players = signal<Record<string, LivePlayer>>({}); | ||
| readonly isFlagFound = signal(false); | ||
| readonly sessionExpired = signal(false); | ||
| readonly MAX_FAILED_ATTEMPTS = 3; | ||
|
|
||
| start(id: string, onConnected: () => void): void { | ||
| start(id: string, onConnected: () => void, onSessionExpired: () => void = () => undefined): void { | ||
| this.stop(); | ||
| this.mode = 'player'; | ||
| this.playerId = id; | ||
| this.onConnected = onConnected; | ||
| this.onSessionExpired = onSessionExpired; | ||
| this.consecutiveFailures = 0; | ||
| this.sessionExpired.set(false); | ||
| this.resume(); | ||
| } | ||
|
|
||
|
|
@@ -72,8 +79,11 @@ export class GameSocketService extends WebSocketService<GameSocketMessage> { | |
| this.mode = null; | ||
| this.playerId = null; | ||
| this.onConnected = null; | ||
| this.onSessionExpired = null; | ||
| this.reconnecting = false; | ||
| this.statusMessage.set(null); | ||
| this.consecutiveFailures = 0; | ||
| this.sessionExpired.set(false); | ||
| this.disconnect(); | ||
| } | ||
|
|
||
|
|
@@ -96,13 +106,33 @@ export class GameSocketService extends WebSocketService<GameSocketMessage> { | |
| this.players.set({}); | ||
| this.reconnecting = false; | ||
| this.statusMessage.set(null); | ||
| this.consecutiveFailures = 0; | ||
| this.onConnected?.(); | ||
| } | ||
|
|
||
| protected override onSocketClose(): void { | ||
| this.reconnecting = true; | ||
| } | ||
|
|
||
| protected override shouldReconnect(closeEvent: CloseEvent): boolean { | ||
| if (this.mode !== 'player') { | ||
| return true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| this.consecutiveFailures++; | ||
|
|
||
| if (this.consecutiveFailures >= this.MAX_FAILED_ATTEMPTS) { | ||
| this.sessionExpired.set(true); | ||
| this.statusMessage.set( | ||
| 'Session has expired as game server restarted.', | ||
| ); | ||
| this.onSessionExpired?.(); | ||
| return false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the trailing whitespace |
||
| } | ||
|
|
||
| return true; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the trailing whitespace |
||
| } | ||
|
|
||
| protected override onSocketError(): void { | ||
| this.statusMessage.set( | ||
| this.mode === 'viewer' | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't support themes in our app.