Remove `event-stream` dependency in favor of plain `Buffers` #1554

astorije · 2017-09-21T07:23:39Z

We were not making real use of event-stream apart for a minor convenience so might as well not rely on it (and its 7 dependencies).

I am not totally familiar with Buffers so I might not be 100% correct, but it does work just as expected and tests pass. 🤷‍♂️

astorije · 2017-09-21T07:24:20Z

src/plugins/irc-events/link.js

-		.pipe(es.wait(function(err, data) {
-			if (err) {
-				return cb(null);
-			}


end's callback has no err argument, so I just removed this. Maybe we need to do this in .on("error", function() {}) above instead?

xPaw · 2017-09-21T11:11:21Z

@astorije I am looking at request code, and it seems to internally buffer as well, but calling abort() destroys it. Maybe calling end() would do the trick, and then in end callback read req.response.body?

astorije · 2017-09-21T14:51:28Z

Sorry, I'm not sure what you mean. Are you talking about never caring about buffers ourselves?

astorije · 2017-09-21T14:53:11Z

Something like this?

──────────────────────────────────────────
modified: src/plugins/irc-events/link.js
──────────────────────────────────────────
@ link.js:145 @ function emitPreview(client, msg, preview) {

 function fetch(uri, cb) {
    let req;
-   let result = new Buffer(0);
    try {
        req = request.get({
            url: uri,
@ link.js:172 @ function fetch(uri, cb) {
            length += data.length;
            if (length > limit) {
                req.response.req.abort();
-           } else {
-               result = Buffer.concat([result, data]);
            }
        })
        .on("end", () => {
@ link.js:191 @ function fetch(uri, cb) {
            }

            cb({
-               data: result,
+               data: req.response.body,
                type: type,
                size: size
            });
```

xPaw · 2017-09-21T14:57:28Z

@astorije Quite possibly like that, but I think abort resets response, so maybe end will work.

astorije · 2017-09-22T05:56:14Z

So, I can't seem to access the body in end handler in any way, any suggestion?

xPaw · 2017-09-22T07:52:08Z

I just tried it myself, and I can't access it either. However I changed the buffering code a little to improve the perf:

If totalLength is not provided, it is calculated from the Buffer instances in list. This however causes an additional loop to be executed in order to calculate the totalLength, so it is faster to provide the length explicitly if it is already known.

And it also no longer drops the last received chunk (it would abort in that, but not buffer it).

diff --git a/src/plugins/irc-events/link.js b/src/plugins/irc-events/link.js
index 06049b1..ef73dc5 100644
--- a/src/plugins/irc-events/link.js
+++ b/src/plugins/irc-events/link.js
@@ -5,7 +5,6 @@ const request = require("request");
 const url = require("url");
 const Helper = require("../../helper");
 const findLinks = require("../../../client/js/libs/handlebars/ircmessageparser/findLinks");
-const es = require("event-stream");
 const storage = require("../storage");
 
 process.setMaxListeners(0);
@@ -143,6 +142,7 @@ function emitPreview(client, msg, preview) {
 
 function fetch(uri, cb) {
 	let req;
+
 	try {
 		req = request.get({
 			url: uri,
@@ -155,8 +155,11 @@ function fetch(uri, cb) {
 	} catch (e) {
 		return cb(null);
 	}
+
+	const buffers = [];
 	var length = 0;
 	var limit = Helper.config.prefetchMaxImageSize * 1024;
+
 	req
 		.on("response", function(res) {
 			if (!(/^image\/.+/.test(res.headers["content-type"]))) {
@@ -166,18 +169,15 @@ function fetch(uri, cb) {
 			}
 		})
 		.on("error", function() {})
-		.pipe(es.map(function(data, next) {
+		.on("data", (data) => {
 			length += data.length;
+			buffers.push(data);
+
 			if (length > limit) {
-				req.response.req.abort();
-			}
-			next(null, data);
-		}))
-		.pipe(es.wait(function(err, data) {
-			if (err) {
-				return cb(null);
+				req.abort();
 			}
-
+		})
+		.on("end", () => {
 			if (req.response.statusCode < 200 || req.response.statusCode > 299) {
 				return cb(null);
 			}
@@ -193,14 +193,12 @@ function fetch(uri, cb) {
 				type = req.response.headers["content-type"].split(/ *; */).shift();
 			}
 
-			data = {
-				data: data,
+			cb({
+				data: Buffer.concat(buffers, length),
 				type: type,
 				size: size
-			};
-
-			cb(data);
-		}));
+			});
+		});
 }
 
 // https://github.com/request/request/issues/2120

astorije · 2017-09-22T14:59:26Z

Good call, will do that.

AlMcKinlay · 2017-09-22T17:48:02Z

This feels like quite a fundamental thing to put into 2.5.0 so close to release, no? We are so close to it.

xPaw · 2017-09-22T18:05:15Z

@YaManicKill It's a simple change, really. I have tested it already with the diff I posted above.

astorije · 2017-09-23T04:16:34Z

I'm perfectly fine with not having it in 2.5.0 (what I intended originally), for what it's worth. Or if you feel like it should, I'm cool with it too. There will be a RC phase anyway and this change is really a "works fine or completely breaks" kind of thing, but really I'm fine with whatever :)

astorije · 2017-09-23T04:38:14Z

Alright, applied your changes @xPaw and it works well.

Also, using the following:

───────────────────────────────────────────
modified: src/plugins/irc-events/link.js
───────────────────────────────────────────
@ link.js:171 @ function fetch(uri, cb) {
 		.on("error", function() {})
 		.on("data", (data) => {
 			if (length + data.length > limit) {
+				console.log("aborting, image too big!");
 				req.abort();
 			} else {
+				console.log("downloading image, current length:", length);
 				length += data.length;
 				buffers.push(data);
 			}
 		})
 		.on("end", () => {
+			console.log("download over, response code:", req.response.statusCode);
 			if (req.response.statusCode < 200 || req.response.statusCode > 299) {
 				return cb(null);
 			}

I could verify that both master and this branch behaved the exact same way.

When successfully downloading an image:

downloading image, current length: 0
downloading image, current length: 1234
[...]
downloading image, current length: 127210
downloading image, current length: 130106
download over, response code: 200

When an image is too big:

downloading image, current length: 0
downloading image, current length: 424
[...]
downloading image, current length: 505671
downloading image, current length: 513863
aborting, image too big!
download over, response code: 200

xPaw · 2017-09-23T07:49:23Z

src/plugins/irc-events/link.js

+				req.abort();
+			} else {
+				length += data.length;
+				buffers.push(data);


You did this differently than in my diff, you're just throwing away last received buffer, remove the else.

you're just throwing away last received buffer

What do you mean? It doesn't matter if we push or not the last chunk, since the request gets aborted and the picture not rendered, does it?

This code is used to fetch HTML too. You're downloading data and throwing it away for no good reason which is a regression compared to previous code.

On that note, we should rename prefetchMaxImageSize into prefetchMaxSize or something, if it's used for non-images too.

It sets the limit to 50kb once it detects a non image.

astorije · 2017-09-23T08:09:15Z

It sets the limit to 50kb once it detects a non image.

Right, I forgot about that. I shouldn't commit at 4am :)

astorije · 2017-09-23T16:08:15Z

@YaManicKill, I'll let you review and decide if you want this in 2.5.0 or later. I won't be offended in any way, do as you prefer :)

AlMcKinlay

Alright, yeah I guess this makes sense. Let's go for it.

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Sep 21, 2017

astorije commented Sep 21, 2017

View reviewed changes

astorije force-pushed the astorije/rm-event-stream branch from 703a7dc to 754debb Compare September 21, 2017 07:38

xPaw added this to the 2.5.0 milestone Sep 22, 2017

astorije force-pushed the astorije/rm-event-stream branch from 754debb to 2233e5b Compare September 23, 2017 04:36

astorije requested review from xPaw and AlMcKinlay September 23, 2017 04:38

dgw mentioned this pull request Sep 23, 2017

Image prefetch: Abort if Content-Length exceeds maximum #1563

Closed

xPaw reviewed Sep 23, 2017

View reviewed changes

Remove event-stream dependency in favor of plain Buffers

75b927b

astorije force-pushed the astorije/rm-event-stream branch from 2233e5b to 75b927b Compare September 23, 2017 08:08

xPaw approved these changes Sep 23, 2017

View reviewed changes

astorije mentioned this pull request Sep 24, 2017

Abort image prefetch if Content-Length exceeds limit #1567

Merged

astorije assigned AlMcKinlay Sep 24, 2017

AlMcKinlay approved these changes Sep 26, 2017

View reviewed changes

AlMcKinlay assigned astorije and unassigned AlMcKinlay Sep 26, 2017

xPaw merged commit 7cfd8d9 into master Sep 26, 2017

xPaw deleted the astorije/rm-event-stream branch September 26, 2017 08:00

astorije changed the title ~~Remove event-stream dependency in favor of plain Buffers~~ Remove event-stream dependency in favor of plain Buffers Sep 29, 2017

astorije changed the title ~~Remove event-stream dependency in favor of plain Buffers~~ Remove event-stream dependency in favor of plain Buffers Sep 29, 2017

astorije changed the title ~~Remove event-stream dependency in favor of plain Buffers~~ Remove event-stream dependency in favor of plain Buffers Sep 29, 2017

astorije added the Meta: Internal This is an internal codebase change (testing, linting, etc.). label Oct 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `event-stream` dependency in favor of plain `Buffers` #1554

Remove `event-stream` dependency in favor of plain `Buffers` #1554

astorije commented Sep 21, 2017

astorije Sep 21, 2017

xPaw commented Sep 21, 2017

astorije commented Sep 21, 2017

astorije commented Sep 21, 2017

xPaw commented Sep 21, 2017

astorije commented Sep 22, 2017

xPaw commented Sep 22, 2017

astorije commented Sep 22, 2017

AlMcKinlay commented Sep 22, 2017

xPaw commented Sep 22, 2017

astorije commented Sep 23, 2017

astorije commented Sep 23, 2017

xPaw Sep 23, 2017

astorije Sep 23, 2017

xPaw Sep 23, 2017

astorije Sep 23, 2017

xPaw Sep 23, 2017

astorije Sep 23, 2017

astorije commented Sep 23, 2017

astorije commented Sep 23, 2017

AlMcKinlay left a comment

Remove event-stream dependency in favor of plain Buffers #1554

Remove event-stream dependency in favor of plain Buffers #1554

Conversation

astorije commented Sep 21, 2017

astorije Sep 21, 2017

Choose a reason for hiding this comment

xPaw commented Sep 21, 2017

astorije commented Sep 21, 2017

astorije commented Sep 21, 2017

xPaw commented Sep 21, 2017

astorije commented Sep 22, 2017

xPaw commented Sep 22, 2017

astorije commented Sep 22, 2017

AlMcKinlay commented Sep 22, 2017

xPaw commented Sep 22, 2017

astorije commented Sep 23, 2017

astorije commented Sep 23, 2017

xPaw Sep 23, 2017

Choose a reason for hiding this comment

astorije Sep 23, 2017

Choose a reason for hiding this comment

xPaw Sep 23, 2017

Choose a reason for hiding this comment

astorije Sep 23, 2017

Choose a reason for hiding this comment

xPaw Sep 23, 2017

Choose a reason for hiding this comment

astorije Sep 23, 2017

Choose a reason for hiding this comment

astorije commented Sep 23, 2017

astorije commented Sep 23, 2017

AlMcKinlay left a comment

Choose a reason for hiding this comment

Remove `event-stream` dependency in favor of plain `Buffers` #1554

Remove `event-stream` dependency in favor of plain `Buffers` #1554