Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Produce a unique namespace at compile time #687

Closed
wants to merge 1 commit into from
Closed

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Oct 18, 2019

So that symbols don't clash when multiple versions of the addon are loaded into node.

Closes #686. Summary of that thread: when two 5.x versions of leveldown are loaded side by side, for example 5.3.0 and 5.0.2, their (static) methods conflict. E.g. if you open a 5.0.2 db, on completion Node.js will call the BaseWorker::Complete method of 5.3.0.

Some remaining questions:

  • Could this be considered a bug in node?
  • Is there a better solution?
  • Only makes leveldown symbols unique, not leveldb symbols
  • I've only tested debug builds so far

@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Oct 18, 2019
vweevers added a commit to vweevers/yarn-macos-pouchdb-error that referenced this pull request Oct 18, 2019
@ralphtheninja
Copy link
Member

ralphtheninja commented Oct 19, 2019

Could this be considered a bug in node?

It seems to me that any native module using the same technique should suffer from same problems.

Only makes leveldown symbols unique, not leveldb symbols

I think this should be fine, since we link statically with leveldb.

@vweevers
Copy link
Member Author

I'll leave this hanging for a few days, in case someone has a better solution, or additional insights.

@vweevers
Copy link
Member Author

@ralphtheninja the leveldb symbols are still exported though, which might cause problems? Similar to how Node.js exporting OpenSSL symbols blocks native addons from using a custom OpenSSL version (nodejs/help#1724).

I'm reading about hiding symbols (-fvisibility=hidden), might help.

@vweevers
Copy link
Member Author

I came across nodejs/node-addon-api#460. These folks recommend always using -fvisibility=hidden for mac. Lemme ask why! It seems to stem from nodejs/node-addon-api#456, which describes a very similar issue 😃

@vweevers
Copy link
Member Author

vweevers commented Oct 19, 2019

Closing in favor of #688.

Don't delete this branch yet because that would break the repro which I'd like to keep around at least until I've tested the prebuilt binaries of the next release.

@vweevers vweevers closed this Oct 19, 2019
vweevers added a commit that referenced this pull request Oct 21, 2019
@vweevers vweevers deleted the add-namespace branch October 21, 2019 22:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-patch Bug fixes that are backward compatible
Projects
None yet
2 participants