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

Use dynamic multithreaded runtime library on windows #127

Merged
merged 1 commit into from
Oct 12, 2024

Conversation

Maxxen
Copy link
Member

@Maxxen Maxxen commented Oct 12, 2024

This fixes duckdb/duckdb-spatial#405

This was quite a journey to debug, and a lot of this windows stuff is new to me, but my understanding of the issue and the fix is that gyp on windows by default compiles node add-ons with a "static runtime library". This means that the native node add-on will not share the heap with any other dynamically loaded libraries (such as DuckDB extensions). When creating an R-Tree in the spatial extension, we hijack the query plan and instantiate a PhysicalCreateRTreeIndex operator, which is a subclass of the PhysicalOperator class present in DuckDB core. This thus gets allocated within the spatial extensions heap, but passed on and ultimately free'd within the DuckDB add-ons heap, causing a crash (?). My understanding is that the fact that we subclass across library boundaries is the key to why this crashes. A similar issue seems to be common when using the static runtime and templates defined in other libraries1.

Note for the future: on debug builds these flags need to change to RuntimeLibrary: 3 and /MDd to link the debug dynamic library instead. e.g.

"msvs_settings": {
      "VCCLCompilerTool": {
          "RuntimeLibrary": 3,
          "ExceptionHandling": 1, 
          "AdditionalOptions": [
              "/bigobj", 
              "/GR",
              "/MDd"
          ]
      }
  }, 

But we don't seem to support separate debug and release targets in this add-on anyway sooo ¯\(ツ)

Some other related threads:

Footnotes

  1. https://stackoverflow.com/questions/35310117/debug-assertion-failed-expression-acrt-first-block-header

Copy link
Collaborator

@carlopi carlopi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks a lot!

@carlopi carlopi merged commit 13cb1d6 into duckdb:main Oct 12, 2024
31 checks passed
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

Successfully merging this pull request may close these issues.

DuckDB Crashes when trying to use r-tree spatial index
2 participants