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

fix possible concurrency in is_threading #318

Closed
wants to merge 4 commits into from
Closed

fix possible concurrency in is_threading #318

wants to merge 4 commits into from

Conversation

lmiq
Copy link
Contributor

@lmiq lmiq commented Jul 3, 2024

This fixes #317

I think the issue is a concurrency that may occur in the is_threading function.

I'm not sure if that function is necessary at all. The issue can be removed by always using the lock, and probably in serial runs the cost of that is irrelevant.

But maybe the bug reflects something deeper, so this might be the wrong fix.

(there is one test failing, but it is not related to this change)

@lmiq lmiq marked this pull request as draft July 3, 2024 20:08
@MarcMush
Copy link
Collaborator

MarcMush commented Jul 3, 2024

Ref #232

@MarcMush
Copy link
Collaborator

MarcMush commented Jul 3, 2024

As a workaround, you can imitate #298

@lmiq
Copy link
Contributor Author

lmiq commented Jul 3, 2024

As a workaround, you can imitate #298

Ok, but this will break possibly many codes in Julia 1.11. A fix is now urgent. I would just do the safe strategy and just always lock, and remove that. Also since tasks can migrate, I have the impression that using threadid() to detect parallelism is not a good idea anyway.

@lmiq lmiq closed this Jul 3, 2024
@lmiq
Copy link
Contributor Author

lmiq commented Jul 3, 2024

Closed because other PRs are dealing with this.

@MarcMush
Copy link
Collaborator

MarcMush commented Jul 3, 2024

I agree the current code is bad and needs to be fixed
Is there a performance hit? If it is non-negligible we can imagine a keyword like thread-safe that defaults to true

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.

Julia 1.11: BoundsError: attempt to access MemoryRef{Int64} at index [1]
2 participants