Replace manual handle memory management with GC allocated space #2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…imer/Poll handles. Fixes use-after-free errors leading to segfaults
This reverts commit 526cb61.
WojciechMazur
commented
Jun 16, 2024
| import internals.HandleUtils | ||
|
|
||
| @inline final class Timer private (private val ptr: Ptr[Byte]) extends AnyVal { | ||
| @inline final class Timer private (private val data: BlobArray) extends AnyVal { |
There was a problem hiding this comment.
Question:
Should we have a registry for active timers? It makes sense for the repeated timers running for the long time in the background. If user does not keep any handle to them, then they can be GCed at some point. How often due users use them in such way?
For safety reasons it might be best to keep them as long as the libUV might trigger the callback. Unfortunetlly we need to bridge 2 memory spaces - the SN space and libUV space.
There was a problem hiding this comment.
Perhaps @lolgab would know the answer?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes the segmentation faults when clearing timer multiple times. There was no way to determine when Timer was already free. On some systems it was fine (MacOS) as the memory was not reused, on others (Ubuntu) it seems like retained memory was reallocated and overridden leading to segmentation fault in a call to
uv_closeNew approach makes sure that as long as we have handle to Timer/Poll the memory would never be retained and overridden. It also allows to skip reference counting to prevent GCing the callbacks.