search

BloomBooks / bloompub-viewer

Desktop Viewer for BloomPUB books
MIT License
4 stars 3 forks source link

Fix unzipping directories (BL-12009) #29

Closed andrew-polk closed 1 year ago

andrew-polk commented 1 year ago

This change is Reviewable

andrew-polk commented 1 year ago

src/main/index.ts line 230 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…
Could you just check that we don't need any sort of locking to protect async code accessing the directories set? Also, are you sure (if node.js proves to be really multithreaded) that all threads will be using the same instance of the set? Since a directory MIGHT just already exist, are you sure mkdirSync won't choke if it does? Assuming none of those questions raise any issues, feel free to merge. LGTM otherwise.

mkdirSync handles the directory already existing without issue.

I searched a bit about multithreading. While it is possible with some tricks to do some multi-threading things in node.js, it seems you can treat it as single-threaded unless you are specifically using those tricks. And as far as I can tell, we are not. Specifically, we are not doing anything like that in our own code.

Even if I'm wrong, the directories set is simply for optimization, so the worst case scenario is that we attempt to create the directory more than once (which will not fail).

Thanks for raising those questions.