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

Requiring tree-sitter from multiple threads causes error #57

Closed
devongovett opened this issue Feb 20, 2020 · 9 comments
Closed

Requiring tree-sitter from multiple threads causes error #57

devongovett opened this issue Feb 20, 2020 · 9 comments

Comments

@devongovett
Copy link

This code causes an error:

require('tree-sitter');
const {Worker} = require('worker_threads');

new Worker('require("tree-sitter")', {eval: true});
Error: Module did not self-register.
    at Object.Module._extensions..node (internal/modules/cjs/loader.js:800:18)
    at Module.load (internal/modules/cjs/loader.js:628:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)
    at Module.require (internal/modules/cjs/loader.js:666:19)
    at require (internal/modules/cjs/helpers.js:16:16)
    at Object.<anonymous> (/Users/govett/Downloads/tree-sitter-test/node_modules/tree-sitter/index.js:3:13)
    at Module._compile (internal/modules/cjs/loader.js:759:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10)
    at Module.load (internal/modules/cjs/loader.js:628:32)
    at Function.Module._load (internal/modules/cjs/loader.js:555:12)

I believe this is due to the module not being context aware, and assuming a single isolate.

I will attempt to look into fixing this issue.

@maxbrunsfeld
Copy link
Contributor

Yeah, unfortunately this module was written before it was possible to have multiple contexts. There are several static variables which will need to be made into struct members, as they describe in those docs.

You may also want to look at #52, which converts the module to use NAPI. I haven't merged it because I haven't had time to test in within Atom yet, but I believe it is passing all tests and feature complete.

@devongovett
Copy link
Author

Ah nice. I tested out #52 and it appears to solve this issue! Looking forward to its release. 😄

@devongovett
Copy link
Author

I'll leave the issue open though, because of those static variables you mentioned. I guess those wouldn't be thread safe.

@maxbrunsfeld
Copy link
Contributor

Just a heads up that it may take some time to get that PR merged, as I'm not working on the Atom team anymore, so it isn't on anybody's high-priority to-do list.

In case you hadn't seen it, another option might be to use the WASM binding to Tree-sitter, web-tree-sitter. I don't think it should have any issues with worker threads.

@cellog
Copy link

cellog commented Feb 14, 2021

@maxbrunsfeld I am trying to use tree-sitter as the parser for a cross-language linter for internationalization internally at Braze. This issue and #72 and tree-sitter/tree-sitter#894 are currently blockers. The reason is we want to run this linter in Docker on PRs, and if it is unstable in a Docker env, that won't work. I already worked around the Jest issue with a weird hack. I tried using the wasm binding, but it runs twice as slow and the embedded template parser throws instantly on attempting to use it. Even ignoring the .erb and .eco templates we have, it took 5 minutes to parse, whereas the node binding took 2:43 to parse everything. I might be able to work with this if it's the only solution, but would need to fix the embedded template parser.

Our project has ruby, coffeescript, typescript plus the erb/eco templates for knockout bindings and i18n can occur in any of those contexts.

Before I sink any more time into this, can you give me an honest appraisal of whether tree-sitter will ever be maintained? It seems like GitHub has dropped Atom, and tree-sitter's raison d'etre was Atom.

I don't want to get to a situation where we have to fork and maintain, as that's just not in the cards. If it looks like there isn't a single source for parsing, I'll have to do a much more complex solution, probably parsers in each language parsing themselves and spitting out an intermediary format, and writing the query logic by hand for each one (ugh).

TIA

@cellog
Copy link

cellog commented Feb 14, 2021

also, I didn't explicitly say this, but I am happy to contribute if it is possible.

@c4lliope
Copy link

@cellog I closed tree-sitter/tree-sitter#894, because my error was very very specific and I imagine your use case may avoid the problem I saw. Please open your own issue if you need!

@maxbrunsfeld
Copy link
Contributor

Before I sink any more time into this, can you give me an honest appraisal of whether tree-sitter will ever be maintained?

Tree-sitter is maintained on a daily basis, but the Node.js binding is currently not. GitHub doesn't use the Node binding for any actively-developed projects, so there's just nobody whose job it is to spend much time on it. The Atom editor does use the Node.js binding, so there is some desire to keep the Node library compatible with existing compiled parsers, as Atom users may have installed several of these as part of Atom extension packages.

@segevfiner
Copy link
Contributor

The module is now Node API based and context aware. This can be closed.

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

6 participants