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.

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

Handle suspend/resume error from watch stream read #2136

Merged
merged 2 commits into from
Feb 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@
"react": "^17.0.1",
"react-dom": "^17.0.1",
"react-router": "^5.2.0",
"readable-web-to-node-stream": "^3.0.1",
"readable-stream": "^3.6.0",
"request": "^2.88.2",
"request-promise-native": "^1.0.8",
"semver": "^7.3.2",
Expand Down Expand Up @@ -277,6 +277,7 @@
"@types/react-router-dom": "^5.1.6",
"@types/react-select": "^3.0.13",
"@types/react-window": "^1.8.2",
"@types/readable-stream": "^2.3.9",
"@types/request": "^2.48.5",
"@types/request-promise-native": "^1.0.17",
"@types/semver": "^7.2.0",
Expand Down
23 changes: 16 additions & 7 deletions src/renderer/api/kube-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import { createKubeApiURL, parseKubeApi } from "./kube-api-parse";
import { KubeJsonApi, KubeJsonApiData, KubeJsonApiDataList } from "./kube-json-api";
import { IKubeObjectConstructor, KubeObject, KubeStatus } from "./kube-object";
import byline from "byline";
import { ReadableWebToNodeStream } from "readable-web-to-node-stream";
import { IKubeWatchEvent } from "./kube-watch-api";
import { ReadableWebToNodeStream } from "../utils/readableStream";

export interface IKubeApiOptions<T extends KubeObject> {
/**
Expand Down Expand Up @@ -373,7 +373,13 @@ export class KubeApi<T extends KubeObject = any> {
opts.abortController = new AbortController();
}
let errorReceived = false;
let timedRetry: NodeJS.Timeout;
const { abortController, namespace, callback } = opts;

abortController.signal.addEventListener("abort", () => {
clearTimeout(timedRetry);
});

const watchUrl = this.getWatchUrl(namespace);
const responsePromise = this.request.getResponse(watchUrl, null, {
signal: abortController.signal
Expand All @@ -387,14 +393,17 @@ export class KubeApi<T extends KubeObject = any> {
}
const nodeStream = new ReadableWebToNodeStream(response.body);

nodeStream.on("end", () => {
if (errorReceived) return; // kubernetes errors should be handled in a callback
["end", "close", "error"].forEach((eventName) => {
nodeStream.on(eventName, () => {
if (errorReceived) return; // kubernetes errors should be handled in a callback
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this comment is wrong. Shouldn't it read something like:

// kubernetes error status events should prevent auto retrying of the watch.


setTimeout(() => { // we did not get any kubernetes errors so let's retry
if (abortController.signal.aborted) return;
clearTimeout(timedRetry);
timedRetry = setTimeout(() => { // we did not get any kubernetes errors so let's retry
if (abortController.signal.aborted) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this give lines 379-381 above, if the signal has already aborted, shouldn't the watch just stop anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, we don't need this.


this.watch({...opts, namespace, callback});
}, 1000);
this.watch({...opts, namespace, callback});
}, 1000);
});
});

const stream = byline(nodeStream);
Expand Down
87 changes: 87 additions & 0 deletions src/renderer/utils/readableStream.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { Readable } from "readable-stream";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { Readable } from "readable-stream";
import { Readable } from "stream";

This package is a mirror of the streams implementations in Node.js.

If you want to guarantee a stable streams base, regardless of what version of Node you, or the users of your libraries are using, use readable-stream only and avoid the "stream" module in Node-core, for background see this blogpost.

Since we bundle app with Node.js v12.x maybe we could use native stream module?
https://nodejs.org/dist/latest-v12.x/docs/api/stream.html#stream_readable_streams

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll do that in a separate PR.


/**
* ReadableWebToNodeStream
*
* Copied from https://github.com/Borewit/readable-web-to-node-stream
*
* Adds read error handler
*
* */
export class ReadableWebToNodeStream extends Readable {

public bytesRead = 0;
public released = false;

/**
* Default web API stream reader
* https://developer.mozilla.org/en-US/docs/Web/API/ReadableStreamDefaultReader
*/
private reader: ReadableStreamReader;
private pendingRead: Promise<any>;

/**
*
* @param stream Readable​Stream: https://developer.mozilla.org/en-US/docs/Web/API/ReadableStream
*/
constructor(stream: ReadableStream) {
super();
this.reader = stream.getReader();
}

/**
* Implementation of readable._read(size).
* When readable._read() is called, if data is available from the resource,
* the implementation should begin pushing that data into the read queue
* https://nodejs.org/api/stream.html#stream_readable_read_size_1
*/
public async _read() {
// Should start pushing data into the queue
// Read data from the underlying Web-API-readable-stream
if (this.released) {
this.push(null); // Signal EOF

return;
}

try {
this.pendingRead = this.reader.read();
const data = await this.pendingRead;

// clear the promise before pushing pushing new data to the queue and allow sequential calls to _read()
delete this.pendingRead;

if (data.done || this.released) {
this.push(null); // Signal EOF
} else {
this.bytesRead += data.value.length;
this.push(data.value); // Push new data to the queue
}
} catch(error) {
this.push(null); // Signal EOF
}
Comment on lines +60 to +62
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the change? Why do we want to signal EOF on an error? Shouldn't we log these errors anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot even catch errors that are thrown from here. If we could I would not have copied this library to our codebase.

All other error cases end up with EOF so it makes sense to push this error to the similar path. I really would like to get this fix into upstream and that means this class itself won't be logging anything.

}

/**
* If there is no unresolved read call to Web-API Readable​Stream immediately returns;
* otherwise will wait until the read is resolved.
*/
public async waitForReadToComplete() {
if (this.pendingRead) {
await this.pendingRead;
}
}

/**
* Close wrapper
*/
public async close(): Promise<void> {
await this.syncAndRelease();
}

private async syncAndRelease() {
this.released = true;
await this.waitForReadToComplete();
await this.reader.releaseLock();
}
}
8 changes: 0 additions & 8 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -11464,14 +11464,6 @@ readable-stream@~1.1.10:
isarray "0.0.1"
string_decoder "~0.10.x"

readable-web-to-node-stream@^3.0.1:
version "3.0.1"
resolved "https://registry.yarnpkg.com/readable-web-to-node-stream/-/readable-web-to-node-stream-3.0.1.tgz#3f619b1bc5dd73a4cfe5c5f9b4f6faba55dff845"
integrity sha512-4zDC6CvjUyusN7V0QLsXVB7pJCD9+vtrM9bYDRv6uBQ+SKfx36rp5AFNPRgh9auKRul/a1iFZJYXcCbwRL+SaA==
dependencies:
"@types/readable-stream" "^2.3.9"
readable-stream "^3.6.0"

readdir-scoped-modules@^1.0.0, readdir-scoped-modules@^1.1.0:
version "1.1.0"
resolved "https://registry.yarnpkg.com/readdir-scoped-modules/-/readdir-scoped-modules-1.1.0.tgz#8d45407b4f870a0dcaebc0e28670d18e74514309"
Expand Down