Skip to content

Extend Windows Process completion key's lifetime#15597

Merged
straight-shoota merged 5 commits intocrystal-lang:masterfrom
HertzDevil:bug/windows-completion-key-lifetime
Mar 28, 2025
Merged

Extend Windows Process completion key's lifetime#15597
straight-shoota merged 5 commits intocrystal-lang:masterfrom
HertzDevil:bug/windows-completion-key-lifetime

Conversation

@HertzDevil
Copy link
Contributor

Alternative to #14995. Fixes #15028.

There is probably a better data structure than Set here...

@HertzDevil HertzDevil added kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:system labels Mar 24, 2025
@HertzDevil HertzDevil moved this to Review in Windows Support Mar 24, 2025
@ysbaddaden
Copy link
Collaborator

Accesses to CompletionKey.all should be protected by a Thread::Mutex when :preview_mt if set.

Copy link
Collaborator

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Thank you 🙇

@ysbaddaden ysbaddaden added this to the 1.16.0 milestone Mar 25, 2025
@straight-shoota
Copy link
Member

I suppose this might be fine for getting the bug fixed, but it seems far from ideal.

We probably shouldn't need this for all CompletionKey instances if it's primarily an issue with Process.
And I'm particularly concerned about @@all becoming a memory leak because it never scales down.

@HertzDevil
Copy link
Contributor Author

That would be a use case for #15494 (so would the Deque used for the fiber stack pool, although that consumes far less memory than the stacks it holds).

I was contemplating some kind of linked list but don't think O(n) deletion is worth it.

@straight-shoota
Copy link
Member

Deletion in a doubly linked list would be O(1) if we have a pointer to the list node. It could be just the CompletionKey instance itself.

@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Mar 27, 2025

Indeed, it's something for Thread::LinkedList with intrusive nodes.

Copy link
Collaborator

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Thank you 🙇

@straight-shoota straight-shoota merged commit 701a27d into crystal-lang:master Mar 28, 2025
34 of 35 checks passed
@github-project-automation github-project-automation bot moved this from Review to Done in Windows Support Mar 28, 2025
@HertzDevil HertzDevil deleted the bug/windows-completion-key-lifetime branch March 30, 2025 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. platform:windows Windows support based on the MSVC toolchain / Win32 API topic:stdlib:system

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Invalid memory access in Windows CI

3 participants