Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usage in multiple worker threads #364

Closed
johanneslumpe opened this issue Mar 27, 2019 · 7 comments
Closed

Usage in multiple worker threads #364

johanneslumpe opened this issue Mar 27, 2019 · 7 comments

Comments

@johanneslumpe
Copy link

Hummus does not work within the context of worker threads in Node 11. More precisely, hummus cannot be loaded within multiple threads. After being loaded more than once it crashes the worker with Error: Module did not self-register..

There is an issue about this here: nodejs/node#21481
It looks like native addons need to be context aware in order to be loaded successfully in multiple threads, as stated here: https://nodejs.org/docs/latest-v10.x/api/addons.html#addons_context_aware_addons

I am not sure how hard it would be to make hummus context aware, but supporting multiple threads would be great.

@johanneslumpe johanneslumpe changed the title Usage in worker threads Usage in multiple worker threads Mar 27, 2019
@galkahana
Copy link
Owner

Hi,
1.0.96 should take care of this.
check it out.

@johanneslumpe
Copy link
Author

johanneslumpe commented Apr 7, 2019

@galkahana thanks for jumping on this! I've just tried it and while hummus now loads in multiple threads, it dies when combining multiple buffers into a final pdf:

#
# Fatal error in , line 0
# Check failed: map->instance_type() == JS_REGEXP_TYPE || map->instance_type() == JS_OBJECT_TYPE || map->instance_type() == JS_ERROR_TYPE || map->instance_type() == JS_ARRAY_TYPE || map->instance_type() == JS_API_OBJECT_TYPE || map->instance_type() == WASM_GLOBAL_TYPE || map->instance_type() == WASM_INSTANCE_TYPE || map->instance_type() == WASM_MEMORY_TYPE || map->instance_type() == WASM_MODULE_TYPE || map->instance_type() == WASM_TABLE_TYPE || map->instance_type() == JS_SPECIAL_API_OBJECT_TYPE.
#
#
#
#FailureMessage Object: 0x700006690c60

Running a single worker works fine, no errors during that operation. As soon as 2 workers are spun up, the above error occurs - not matter if a single instance or multiple instances do work. Could this be related to hummus accessing data that is somehow shared between all module instances and it has to use local versions of the types to check against? Similar to how in JS we cannot do instanceof across different host environments? In my case the error occurred when using pdfWriter.appendPDFPagesFromPDF to combine multiple PDFs.

@galkahana
Copy link
Owner

Not sure. PDFWriter is supposed to be thread safe in that it does not share objects with other PDFWriters. so using it within one thread, and another one within another should be fine. but i don't know if "combining buffer into a final pdf" goes into this definition. depending on what objects are used accross threads, we might have a sharing issue here (for instance, the internal object numbers allocator is single to a single instance of PDFWriter. if multiple objects are allocated at the same time...could create a problem.
As for the NodeJS wrappers. I think they are done right so that no JS objects are shared between different contexts. but maybe im wrong.

better check out the usage. wanna share a sample script? i could debug it, or see where it may be breaching what sharing issues im aware of.

Gal.

@johanneslumpe
Copy link
Author

Thanks for keeping an eye on this! I will try to share a repo with a runnable example tonight!

@johanneslumpe
Copy link
Author

Here is the promised repo: https://github.com/johanneslumpe/hummus-worker-threads-bug
The tests were run on node 11.11.0

I've noticed that I am also getting Illegal instruction: 4 after the crash, but I am not sure if that is just cropping up because of the failure. Curious to see how the test cases behave for you.

@galkahana
Copy link
Owner

K. actually i totally missed that i had quite a lot of globally shared (and with threads overridden incorrectly) elements. Had to be with how the node drivers for the C++ object are built.
Anyways, quite a bit of work, but now with 1.0.99 it should be fine. I checked your code, and my own tests RE threads.

@johanneslumpe
Copy link
Author

I just confirmed that the test cases in the repo work fine now! Awesome work, thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants