Skip to content

[backport 1.11] Set array size only when safe to do so #58847

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

Closed

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Jun 29, 2025

I looked through the _growend! function from the stacktrace here and noticed that the .size of an array might be getting set too early; before the memory was actually allocated. Maybe there are very specific GC conditions (juliacall + macOS) where this segfaults things.

Possible fix for #58475 #58171. Edit: nope, this wasn't the fix. But seems like a good thing to have anyways so I'll submit it regardless

@MilesCranmer MilesCranmer changed the title Set array size only when safe to do so [backport 1.11] Set array size only when safe to do so Jun 29, 2025
@bvdmitri
Copy link
Contributor

Do you mean potential race-conditions with GC when saying that the current behaviour sets size "too early"? If this is the case, the proposed behavior is still prone to race conditions, but it will be "too late" instead because the memory has been allocated but the size has not been updated yet. Those two fields really should be set together atomically, the actual order should not be relevant

@MilesCranmer
Copy link
Member Author

I agree. BUT since they aren't atomic, this order is the least bad.

If the array shrinks, then you modify the length immediately. If the array grows, then you modify the length at the end.

This lowers the chance of (but does not completely remove) corruption from thread race.

I don't feel strongly though. Feel free to take or reject

@mbauman
Copy link
Member

mbauman commented Jun 30, 2025

Let's tackle #58848 first and then come back to backport it after that is merged. You should be able to re-open this one at that point... if a manual backport is even necessary (if it applies cleanly then it can be automatically hoovered up into a batch of backports in one go).

@mbauman mbauman closed this Jun 30, 2025
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.

3 participants