openzim / node-libzim

Libzim binding for Node.js: read/write ZIM files in Javascript
https://www.npmjs.com/package/@openzim/libzim
GNU General Public License v3.0
27 stars 11 forks source link

Code Review #8

Closed ISNIT0 closed 5 years ago

ISNIT0 commented 5 years ago

Hi @mgautierfr, I would really appreciate your review of this CPP code. I have very little knowledge of best practices, and I feel there are likely to be silly errors!

(CC'd: @kelson42 )

mgautierfr commented 5 years ago

https://github.com/openzim/node-libzim/blob/e41231d929e5c1442bdf52a5235584b7371857be/src/ZimReader.cc#L22-L27

This would means that there is only one ZimReaderWrapper. The static inside the method means that rw is stored in a static memory (ie a global memory common to all "call" to create). On a more global pov, why the wrapper is needed at all ? It simply store a zim::File and does nothing as all the code is in the zimReaderManager and directly use rw->_reader. You would better to remove the wrapper and make zimReaderManager directly interacte with a zim::File instead of a ZimReaderWrapper. Or move the code in the wrapper and have the manager only managing wrapper (create them, delete them) and not doing interaction with the thing wrapped. (But in this case, you can simply have a simple function to create the wrapper, no need for a class)

This is the same problem with the ZimCreatorWrapper


https://github.com/openzim/node-libzim/blob/e41231d929e5c1442bdf52a5235584b7371857be/src/Article.cc#L128

You have to be carreful here. I don't know how nbind and node are doing it, but you are not taking any reference to the buffer buf. If you lose all reference in the js word, the gc will clean buf object and you will have dangling pointers to some random memory.


Be carreful, zimCreator is not thread safe. You must use it from only one thread. There is a lot of promise, async in your js code and I'm not sure this is compatible.

ISNIT0 commented 5 years ago

The wrappers are needed for compatibility with nbind unfortunately. I think both of these suggestions are not possible with nbind 😢

This is all using only one pointer, the Promise and async are in-fact just single threaded 😄

mgautierfr commented 5 years ago

The wrappers are needed for compatibility with nbind unfortunately. I think both of these suggestions are not possible with nbind cry

Why do you need two classes ? The manager and the wrapper could be the same class. Even more if there is only one creator/reader.

mgautierfr commented 5 years ago

You must also destruct the zimCreator (and Reader) at a moment. (delete cw._c) Else the temporary files are not deleted. (See openzim/mwoffliner#568)