Skip to content

Commit

Permalink
fix(client): Some close events are not worth retrying
Browse files Browse the repository at this point in the history
  • Loading branch information
enisdenjo committed Nov 14, 2020
1 parent 7cebbfe commit 4d9134b
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 48 deletions.
81 changes: 48 additions & 33 deletions src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,48 @@ export function createClient(options: ClientOptions): Client {
}),
];
}
/**
* Checks the `connect` problem and evaluates if the client should
* retry. If the problem is worth throwing, it will be thrown immediately.
*/
function shouldRetryConnectOrThrow(errOrCloseEvent: unknown): boolean {
// throw non `CloseEvent`s immediately, something else is wrong
if (!isCloseEvent(errOrCloseEvent)) {
throw errOrCloseEvent;
}

// some close codes are worth reporting immediately
if (
[
1002, // Protocol Error
1011, // Internal Error
4400, // Bad Request
4401, // Unauthorized (tried subscribing before connect ack)
4409, // Subscriber for <id> already exists (distinction is very important)
4429, // Too many initialisation requests
].includes(errOrCloseEvent.code)
) {
throw errOrCloseEvent;
}

// normal closure is disposal, shouldnt try again
if (errOrCloseEvent.code === 1000) {
return false;
}

// user cancelled early, shouldnt try again
if (errOrCloseEvent.code === 3499) {
return false;
}

// retries are not allowed or we tried to many times, report error
if (!retryAttempts || state.tries > retryAttempts) {
throw errOrCloseEvent;
}

// looks good, please retry
return true;
}

// in non-lazy (hot?) mode always hold one connection lock to persist the socket
if (!lazy) {
Expand All @@ -419,22 +461,11 @@ export function createClient(options: ClientOptions): Client {
// cancelled, shouldnt try again
return;
} catch (errOrCloseEvent) {
// throw non `CloseEvent`s immediately, something else is wrong
if (!isCloseEvent(errOrCloseEvent)) {
throw errOrCloseEvent; // TODO-db-200909 promise is uncaught, will appear in console
}

// normal closure is disposal, shouldnt try again
if (errOrCloseEvent.code === 1000) {
return;
}

// retries are not allowed or we tried to many times, close for good
if (!retryAttempts || state.tries > retryAttempts) {
// return if shouldnt try again
if (!shouldRetryConnectOrThrow(errOrCloseEvent)) {
return;
}

// otherwise, wait a bit and retry
// if should try again, wait a bit and continue loop
await new Promise((resolve) => setTimeout(resolve, retryTimeout));
}
}
Expand Down Expand Up @@ -533,27 +564,11 @@ export function createClient(options: ClientOptions): Client {
// cancelled, shouldnt try again
return;
} catch (errOrCloseEvent) {
// throw non `CloseEvent`s immediately, something else is wrong
if (!isCloseEvent(errOrCloseEvent)) {
throw errOrCloseEvent;
}

// normal closure is disposal, shouldnt try again
if (errOrCloseEvent.code === 1000) {
// return if shouldnt try again
if (!shouldRetryConnectOrThrow(errOrCloseEvent)) {
return;
}

// user cancelled early, shouldnt try again
if (errOrCloseEvent.code === 3499) {
return;
}

// retries are not allowed or we tried to many times, close for good
if (!retryAttempts || state.tries > retryAttempts) {
throw errOrCloseEvent;
}

// otherwise, wait a bit and retry
// if should try again, wait a bit and continue loop
await new Promise((resolve) => setTimeout(resolve, retryTimeout));
}
}
Expand Down
87 changes: 72 additions & 15 deletions src/tests/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -640,12 +640,16 @@ describe('reconnecting', () => {
it('should not reconnect if retry attempts is zero', async () => {
const { url, ...server } = await startTServer();

createClient({
url,
lazy: false,
retryAttempts: 0,
retryTimeout: 5, // fake timeout
});
const sub = tsubscribe(
createClient({
url,
retryAttempts: 0,
retryTimeout: 5, // fake timeout
}),
{
query: 'subscription { ping }',
},
);

await server.waitForClient((client) => {
client.close();
Expand All @@ -654,17 +658,26 @@ describe('reconnecting', () => {
await server.waitForClient(() => {
fail('Shouldnt have tried again');
}, 20);

// client reported the error
await sub.waitForError((err) => {
expect((err as CloseEvent).code).toBe(1005);
});
});

it('should reconnect silently after socket closes', async () => {
const { url, ...server } = await startTServer();

createClient({
url,
lazy: false,
retryAttempts: 1,
retryTimeout: 5,
});
const sub = tsubscribe(
createClient({
url,
retryAttempts: 1,
retryTimeout: 5,
}),
{
query: 'subscription { ping }',
},
);

await server.waitForClient((client) => {
client.close();
Expand All @@ -679,6 +692,48 @@ describe('reconnecting', () => {
await server.waitForClient(() => {
fail('Shouldnt have tried again');
}, 20);

// client reported the error
await sub.waitForError((err) => {
expect((err as CloseEvent).code).toBe(1005);
});
});

it('should report some close events immediately and not reconnect', async () => {
const { url, ...server } = await startTServer();

async function testCloseCode(code: number) {
const sub = tsubscribe(
createClient({
url,
retryAttempts: Infinity, // keep retrying forever
retryTimeout: 5,
}),
{
query: 'subscription { ping }',
},
);

await server.waitForClient((client) => {
client.close(code);
});

await server.waitForClient(() => {
fail('Shouldnt have tried again');
}, 20);

// client reported the error
await sub.waitForError((err) => {
expect((err as CloseEvent).code).toBe(code);
});
}

await testCloseCode(1002);
await testCloseCode(1011);
await testCloseCode(4400);
await testCloseCode(4401);
await testCloseCode(4409);
await testCloseCode(4429);
});

it.todo(
Expand All @@ -698,7 +753,6 @@ describe('events', () => {
const client = await new Promise<Client>((resolve) => {
const client = createClient({
url,
lazy: false,
retryAttempts: 0,
on: {
connecting: connectingFn,
Expand All @@ -712,9 +766,12 @@ describe('events', () => {
resolve(client);
});
client.on('closed', closedFn);

// trigger connecting
tsubscribe(client, { query: 'subscription {ping}' });
});

expect(connectingFn).toBeCalledTimes(1); // only once because `client.on` missed the initial connecting event
expect(connectingFn).toBeCalledTimes(2);
expect(connectingFn.mock.calls[0].length).toBe(0);

expect(connectedFn).toBeCalledTimes(2); // initial and registered listener
Expand All @@ -738,7 +795,7 @@ describe('events', () => {
}

// retrying is disabled
expect(connectingFn).toBeCalledTimes(1);
expect(connectingFn).toBeCalledTimes(2);
expect(connectedFn).toBeCalledTimes(2);

expect(closedFn).toBeCalledTimes(2); // initial and registered listener
Expand Down

0 comments on commit 4d9134b

Please sign in to comment.