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

Fix flush() to retry fetch() on transient errors #9

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix flush() to retry fetch() on transient errors #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 24 additions & 2 deletions src/http/stream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,8 +329,7 @@ export class HttpStream extends Stream implements SqlOwner {
let promise;
try {
const request = createRequest();
const fetch = this.#fetch;
promise = fetch(request);
promise = this.#fetchWithRetry(request);
} catch (error) {
promise = Promise.reject(error);
}
Expand All @@ -356,6 +355,20 @@ export class HttpStream extends Stream implements SqlOwner {
});
}

#fetchWithRetry(request: Request, retryCount: number = 3, backoff: number = 100): Promise<Response> {
const fetch = this.#fetch;
return fetch(request).catch((error) => {
if (isRetryableError(error)) {
Copy link

Choose a reason for hiding this comment

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

Will that really solve the issue?
Don't you need to in the catch of the promise here

hrana-client-ts/src/http/stream.ts

Line 350 in 5d5bcdf

this._setClosed(error);
?

Unless fetch throws that error synchronously (I kinda doubt it does), I think it will escape your solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mateusfreira I think that point is too late to safely retry because it covers things like failing to process the response. For example, we may have successfully executed a INSERT statement and committed it to the database, but fail later on. Retrying would mean a duplicate insert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mateusfreira I did switch to catching errors from the promise returned by fetch(), though, and added exponential backoff.

if (retryCount > 0) {
return new Promise((resolve) => setTimeout(resolve, backoff)).then(() => {
return this.#fetchWithRetry(request, retryCount - 1, backoff * 2);
});
}
}
return Promise.reject(error);
});
}

#createPipelineRequest(pipeline: Array<PipelineEntry>, endpoint: Endpoint): Request {
return this.#createRequest<proto.PipelineReqBody>(
new URL(endpoint.pipelinePath, this.#baseUrl),
Expand Down Expand Up @@ -417,6 +430,15 @@ export class HttpStream extends Stream implements SqlOwner {
}
}

function isRetryableError(error: any): boolean {
if (!error.errno) {
return false;
}
return error.errno === "EPIPE"
|| error.errno === "ECONNREFUSED"
|| error.errno === "ECONNRESET";
}

function handlePipelineResponse(pipeline: Array<PipelineEntry>, respBody: proto.PipelineRespBody): void {
if (respBody.results.length !== pipeline.length) {
throw new ProtoError("Server returned unexpected number of pipeline results");
Expand Down
Loading