Incorrect stream handling in Node v14

[benabel]

Describe the bug
When running the example provided, I can't upload my contents taken from here:

https://exo.lyceum.fr/node/1

Environment:

• OS: linux
• Browser firefox
• Nodejs-Version 14.2.0

To Reproduce
Steps to reproduce the behavior:

1. Go to http://localhost:8080/h5p/new
2. Click on Upload a file.
3. Click on Use
4. See the app hanging on Loading, please wait...

Screenshots

[sr258]

I've downloaded the h5p package from the website and it does work in my test setup:

Does the problem also exist with other content (or content types)?

Possibly, something went wrong when downloading the H5P core files or some npm dependency. Can you please try doing the following:

npm run clear
rm -rf h5p

Then re-install the dependencies and the h5p library with

npm run install

[benabel]

Thanks for your reply, so far I've tested:

npm run clear && rm -rf h5p && npm install && npm start

and:

npm run clear && rm -rf h5p && npm install && npm run build && npm start

Both do not work, I have this error in the console:

[benabel]

When I investigate in chromium console I can see that some requests are never fullfilled staying on pending. For every content type upload or new one.

However each of these requests works when coîed as curl commands.

[sr258]

The first 404 error is ok, it's not the reason you see the error. The actual problem is that all the JavaScript files are nt loaded by the browser.

Can you please try running the example like this:

DEBUG=h5p* LOG_LEVEL=debug npm start

Then you'll receive more detailled debug information in the console.

You can display even more log entries (the ones generated by express) with this command:

DEBUG=* LOG_LEVEL=debug npm start

[benabel]

I tried the two commands above without any information, the app hangs on this request:

 h5p:LibraryManager getting file js/multichoice.js from library H5P.MultiChoice-1.14 +1ms

So js files can't be fetched.

I tried the tests and e2e:tests and they pass.

[benabel]

Ok I finnaly found what is going on. It is due to the node version. I tried in another machine running node v10 or v12 and everything is ok. When I switch to node v14 I can't use almost any content with the js fetches never fulfilled.

I think the e2e tests don't test the new route and that is why all tests pass on node v14.

[sr258]

Thank you for your very helpful investigation and sorry you had all this trouble. I‘ll investigate why Node 14 breaks things soon.

[sr258]

I've identified the cause of the issue: It seems like subscribing to the 'end' event in the Express controller breaks piping the stream to Express. You have to remove

        stream.on('end', () => {
            res.end();
        });

from ExpressController.getLibraryFile. However, I'm pretty certain that I've inserted these lines because it didn't work without it in the past. I'll have to read a bit about piping streams in Express to be able to write a fix. This might also apply to other endpoints that return streams. If anyone out there knows why and how the behaviour of streams has changed in Node v14, I'd appreciate some clue.

[benabel]

At the moment I have downgraded to node v12. Thanks for your investigations, I haven't seen any deprecation on node 12 that might help to figure out what is going on.

[benabel]

Maybe this commit

nodejs/node@ae157b8ca7

[benabel]

After some investigations, it seems that this caused by this commit:

nodejs/node#32968

Changing 'end' event with 'close' event seems to fix the problem on v14 and v12 is still ok.

Maybe a e2e test should be added for /new route.

[sr258]

I agree, nodejs/node#32968 seems to be the culprit. However, the issue is supposed to be resolved with Node 14.1.0. I've tried it with Node 14.2.0 and it still doesn't work. It look like we have to fix this here.

I now remember why I added the subscription in the first place: It is a speed optimization. Without ending the Express response stream manually on the file stream's 'end', requests simply took longer to complete. As the H5P client requests a lot of files sequentially, these few milliseconds per request added up to a noticable performance decrease. It looks like we'll have to sacrifice this speed optimization for the time being, as Node 14 compatibility is certainly something we should strive for. It's probably better to optimize by packing files into bundles, anyway. @JPSchellenberg what do you think?

I agree about the e2e tests. I've had a bad feeling about their state for some time now. They should be redone in general, as they only test the plaer and basically use their own example server implementation, which we should scrap for the example implementation, which has become pretty comprehensive. They also requires a GUI to run, which makes it difficult to run on GitHub actions. So rewriting and extending them is something that should be done soon.

[sr258]

This might have been fixed by a recent PR merge, in which stream piping was changed. @benabel can you confirm that this now works in your setup?

[benabel]

That's fixed on my machine too, I tried v3.0.3 and master I can now add contents with both node 12 and 14. Thank you for your support @sr258, I close the issue.

[sr258]

Regression in (private) ajax adapter branch