Migrate from readable-web-to-node-stream to readable-from-web

[surilindur]

After dropping support for Node 16, it is now possible to also drop the library used to convert Web ReadableStreams to Node's own ReadableStreams by using the built-in Readable.fromWeb function that was added in Node 17.

This helps both reduce the number of dependencies and, most critically, avoid the uncaught errors when the stream is aborted, such as when the SPARQL endpoint crashes for whatever reason. This finally fixes the SPARQL benchmark runner crashing when the server crashes, it only took two weeks or so to figure it out. 😢

[coveralls]

Pull Request Test Coverage Report for Build 8427459576

Details

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 8372373746:	0.0%
Covered Lines:	83
Relevant Lines:	83

---

💛 - Coveralls

[rubensworks]

We can't depend on node built-ins unfortunately, as we can't use those in the browser.

Perhaps this functionality also exists in the readable-stream package?

[rubensworks]

So the Readable.fromWeb does not exist in readable-stream?

If not, to what extent does the code below correspond to Node's Readable.fromWeb impl? Just to know how stable and tested the code below is.

[surilindur]

Yes, the Readable.fromWeb does not exist in readable-stream. They have a from that supports AsyncInterable and something else, but not the response body directly as-is.

The code does approximately the same thing as Node's own implementation here in the read method, except it does not destroy the stream upon error, just emits the error as an event.

It seems to work the same way as Readable.fromWeb with regards to successful and failed streams, based on testing with the SPARQL benchmark runner and the unit test. The new unit test I added makes sure the errors can be handled through the error events in the converted Readable stream, when they were previously uncaught.

I do not know how to further test this, so any tips would be welcome.

[rubensworks]

Could you also manually test this in a browser build of comunica targeting a SPARQL endpoint?
The easiest would probably be to link this within the comunica jquery widget, and then sending some queries to sparql endpoints.

[surilindur]

I will try to reimplement the entire fromWeb function using Node's as reference. No idea how it will work.

[rubensworks]

Can't we just stick with readable-web-to-node-stream then? We know that works. No need to re-invent the wheel, unless you see a good reason?

[surilindur]

The readable-web-to-node-stream library does not handle errors at all, and it crashes the whole program with an uncaught error exception when the web stream errors out, such as when the server terminates the connection or the undici body timeout is reached. That is what also causes the SPARQL benchmark runner to crash when the SPARQL endpoint crashes or otherwise closes the connection.

[surilindur]

I could look into fixing the fork of readable-web-to-node-stream next week, I guess. This uncaught error has already taken two weeks or more, so spending a day or two on fixing that should not be that bad.

[rubensworks]

The readable-web-to-node-stream library does not handle errors at all, and it crashes the whole program with an uncaught error exception when the web stream errors out

Ok, that's a good point indeed.

Fixing the fork then sounds like a good idea. Perhaps you could even create your own fork (maybe we even want it under the comunica namespace)?
If I remember correctly, readable-web-to-node-stream is used in another place in comunica, so having that reusable fork would be good.

[surilindur]

There is now the library under Comunica that handles this conversion without crashing everything, and I have updated the code in this PR to use that, instead.

[rubensworks]

Nice work!

Before we merge, can you confirm to have linked this to the comunica jquery, built it for the browser, and tried out a SPARQL query sent to a SPARQL endpoint, to manually check if things still work correctly in the browser?

[rubensworks]

I apologise, but I must backtrack my previous comment.
We'll want the return type here to remain NodeJS.ReadableStream to avoid breaking changes in the API. (but in practise, both should be usable interchangeably)

[surilindur]

Sounds reasonable. 🙂 I have reverted the changes now, leaving only the function-internal one (to replace the library) and a cast in fetchTriples after swapping the Readable to come from readable-stream (it was already returning that one, as opposed to NodeJS.ReadableStream).

Do the changes look acceptable now?

[surilindur]

Before we merge, can you confirm to have linked this to the comunica jquery, built it for the browser, and tried out a SPARQL query sent to a SPARQL endpoint, to manually check if things still work correctly in the browser?

I finally managed to add browser tests to the readable-from-web library for Firefox, Chromium and WebKit, and those pass after using webpack to bundle the library itself for browser use. This means that the new replacement library works for converting streams in those browsers, as well.

If still needed, I can do the linking and testing with the jQuery widget, but I think it will work just fine in a browser, since the old library also did. 🤔

[rubensworks]

If still needed, I can do the linking and testing with the jQuery widget, but I think it will work just fine in a browser, since the old library also did. 🤔

I would definitely do this. Out of personal experience I know that unexpected breakages easily happen when working with Node-browser environments.

[surilindur]

I linked the version from this branch to Comunica (v3 master), and linked that Comunica to the jQuery widget, built it, and tested with the DBPedia SPARQL endpoint without issues. 🎉

[rubensworks]

Thanks @surilindur!

I'lll wait to release a new version until #79 is resolved.

Could you also look into applying this change in comunica's ActorHttp?