Skip to content

Conversation

@peterzhu2118
Copy link
Member

As @jeremyevans pointed out for commit eb2d8b1:

Each Tempfile instance has a separate File instance and file descriptor:

t = Tempfile.new
t.to_i # => 6
t.dup.to_i => 7

FinalizerManager will keep track of the open File objects for the particular file and will only unlink the file when all of the File objects have been closed.

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

Is there a reason for @finalizer_obj if you are going to have a separate object per Tempfile? It seems better to just use self instead of @finalizer_obj in that case.

Please add the pid handling back or provide a justification for why you think we should remove it (it's been there since 2001). If you want to remove the $DEBUG printing, I'm OK with that (no justification required), but my preference would be to keep it as well.

Otherwise, this looks good.

@peterzhu2118 peterzhu2118 force-pushed the pz-finalizer-manager branch 2 times, most recently from fb4da22 to 88ed09c Compare August 20, 2024 17:59
As @jeremyevans pointed out for commit eb2d8b1:

> Each Tempfile instance has a separate File instance and file descriptor:
>
>   t = Tempfile.new
>   t.to_i # => 6
>   t.dup.to_i => 7

FinalizerManager will keep track of the open File objects for the
particular file and will only unlink the file when all of the File objects
have been closed.
@peterzhu2118
Copy link
Member Author

Is there a reason for @finalizer_obj if you are going to have a separate object per Tempfile? It seems better to just use self instead of @finalizer_obj in that case.

The downside of this is that any user defined finalizers on this could be lost because we sometimes need to undefine the finalizers on the object. But looking at the original code, they already undefine finalizer on the Tempfile object, so it shouldn't be a big issue. I've changed it to use self.

Please add the pid handling back

That was just lost in refactoring, I've added it back. I've also restored the debug output.

@peterzhu2118 peterzhu2118 merged commit 4ad02f0 into master Aug 20, 2024
@peterzhu2118 peterzhu2118 deleted the pz-finalizer-manager branch August 20, 2024 18:07
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