Skip to content

Commit 19c6489

Browse files
fix(newsfeed): prevent duplicate parse error callback when using pipeline (#4083)
In PR #4072 GitHub Bot complained that `newsfeedfetcher.js` used the old `.pipe()` method to connect streams (HTTP body → iconv decoding → RSS parser). `.pipe()` has a weakness: errors in one stream are **not** automatically forwarded to downstream streams. An I/O or decoding error would silently disappear. ## Solution Replaced `.pipe()` with `await stream.promises.pipeline()`. The `pipeline()` API is designed to propagate errors correctly through the entire chain and to clean up all streams on failure. Errors now reliably land in the `catch` block and call `fetchFailedCallback` exactly once. The redundant `parser.on("error")` handler was removed, as it would have caught the same error again and called the callback a second time. ## Why not the bot's suggested fix? The GitHub Bot suggested the older callback-based `stream.pipeline(callback)` variant: ```js stream.pipeline(nodeStream, iconv.decodeStream(...), parser, (error) => { if (!error) return; // error handling... }); ``` This has two drawbacks compared to my approach: 1. It uses the older callback style — `stream.promises.pipeline()` with `async/await` is the modern, more readable API. 2. The bot's suggestion kept both the `parser.on("error")` handler **and** the `catch` block, which would not have fixed the double-callback problem. ---- Related to #4073
1 parent d2304af commit 19c6489

1 file changed

Lines changed: 3 additions & 8 deletions

File tree

defaultmodules/newsfeed/newsfeedfetcher.js

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class NewsfeedFetcher {
4040
});
4141

4242
// Wire up HTTPFetcher events
43-
this.httpFetcher.on("response", (response) => this.#handleResponse(response));
43+
this.httpFetcher.on("response", (response) => void this.#handleResponse(response));
4444
this.httpFetcher.on("error", (errorInfo) => this.fetchFailedCallback(this, errorInfo));
4545
}
4646

@@ -67,7 +67,7 @@ class NewsfeedFetcher {
6767
* Handles successful HTTP response
6868
* @param {Response} response - The fetch Response object
6969
*/
70-
#handleResponse (response) {
70+
async #handleResponse (response) {
7171
this.items = [];
7272
const parser = new FeedMe();
7373

@@ -106,11 +106,6 @@ class NewsfeedFetcher {
106106

107107
parser.on("end", () => this.broadcastItems());
108108

109-
parser.on("error", (error) => {
110-
Log.error(`${this.url} - Feed parsing failed: ${error.message}`);
111-
this.fetchFailedCallback(this, this.#createParseError(`Feed parsing failed: ${error.message}`, error));
112-
});
113-
114109
parser.on("ttl", (minutes) => {
115110
const ttlms = Math.min(minutes * 60 * 1000, 86400000);
116111
if (ttlms > this.httpFetcher.reloadInterval) {
@@ -123,7 +118,7 @@ class NewsfeedFetcher {
123118
const nodeStream = response.body instanceof stream.Readable
124119
? response.body
125120
: stream.Readable.fromWeb(response.body);
126-
nodeStream.pipe(iconv.decodeStream(this.encoding)).pipe(parser);
121+
await stream.promises.pipeline(nodeStream, iconv.decodeStream(this.encoding), parser);
127122
} catch (error) {
128123
Log.error(`${this.url} - Stream processing failed: ${error.message}`);
129124
this.fetchFailedCallback(this, this.#createParseError(`Stream processing failed: ${error.message}`, error));

0 commit comments

Comments
 (0)