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 API lock in helper function ccalls #1181

Merged
merged 4 commits into from
Nov 20, 2024
Merged

use API lock in helper function ccalls #1181

merged 4 commits into from
Nov 20, 2024

Conversation

lxvm
Copy link
Contributor

@lxvm lxvm commented Nov 18, 2024

Hi,

This pr wraps ccalls in src/api/helpers.jl with the library API lock. I believe the handling of strings is still thread-safe, but it would help for someone to check. Also, if this pr should be implemented at the level of the generator I may need some assistance with where to start.

Fixes #1179

@mkitti
Copy link
Member

mkitti commented Nov 18, 2024

Thank you. This is probably the right direction. Does applying this lock solve the issue in #1179 for you? Could we also implement a minimal version of the MWE you reported in #1179 as a test?

@lxvm
Copy link
Contributor Author

lxvm commented Nov 19, 2024

I checked that this pr fixes the issue in #1179 on my computer. To turn that MWE into a test, would I have to add a new test suite in a multi-threaded Julia process? I don't see other multi-threaded tests so I'm not sure what the best way to write such a test would be.

For the formatter, I'll try and fix the failing test soon.

@mkitti
Copy link
Member

mkitti commented Nov 20, 2024

For the formatter you can run contrib/format/format.jl and push enter until it says it was formatted.

@mkitti
Copy link
Member

mkitti commented Nov 20, 2024

For a multiple thread test, you can launch another Julia process as follows.

run(`$(Base.julia_cmd()) -t 2 -E "Threads.nthreads()"`)

Instead of -E you could just provide a file to run.

@lxvm
Copy link
Contributor Author

lxvm commented Nov 20, 2024

Thanks for the recommendations. I added a test similar to what you described. I think it uses Pkg.jl internals to get a path to the test environment, which may be undesirable, but the test does show the MWE gets fixed by this pr when run on my machine. CI shows it passes on all platforms and versions except for nightly, but I'm not sure why this fix would segfault on nightly.

@mkitti
Copy link
Member

mkitti commented Nov 20, 2024

Close enough for me. I'll merge and investigate the failures further or mark them as broken.

@mkitti mkitti merged commit ebaaeee into JuliaIO:master Nov 20, 2024
20 of 24 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.

threaded segfault in setindex! with arrays of compound-typed elements
2 participants