Handle suspend/resume error from watch stream read #2136
Conversation
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
| @@ -0,0 +1,87 @@ | |||
| import { Readable } from "readable-stream"; | |||
There was a problem hiding this comment.
| 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
There was a problem hiding this comment.
Makes sense, I'll do that in a separate PR.
| 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; |
There was a problem hiding this comment.
Why do we need this give lines 379-381 above, if the signal has already aborted, shouldn't the watch just stop anyway?
There was a problem hiding this comment.
You are right, we don't need this.
| } catch(error) { | ||
| this.push(null); // Signal EOF | ||
| } |
There was a problem hiding this comment.
This is the change? Why do we want to signal EOF on an error? Shouldn't we log these errors anyway?
There was a problem hiding this comment.
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 (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 |
There was a problem hiding this comment.
I think this comment is wrong. Shouldn't it read something like:
// kubernetes error status events should prevent auto retrying of the watch.
Had to copy
ReadableWebToNodeStreamto here because we cannot catch read errors with the upstream version. We should probably contribute a fix to upstream.Fixes #2133