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 Log deletion bug #25

Merged
merged 1 commit into from
Jul 20, 2020
Merged

Fix Log deletion bug #25

merged 1 commit into from
Jul 20, 2020

Conversation

schneems
Copy link
Member

@schneems schneems commented Jul 20, 2020

A Tempfile works by defining a "finalizer" this is code that gets run when the object is cleaned up via GC. Here's a link:

https://github.com/ruby/ruby/blob/935d0b3d05dfc8b30bba505792129bf0e33ebe3b/lib/tempfile.rb#L132

As long as another object has a reference to a Tempfile then it will not be deleted from disk. In this scenario we're passing it to a Pathname object and it does not retain a reference to the passed in argument. You can reproduce this bug/behavior outside of Rundoc:

$ cat scratch.rb
require 'pathname'
require 'tempfile'

def make_pathname(value = Tempfile.new("log"))
  Pathname.new(value)
end

p = make_pathname
FileUtils.touch(p)

puts "File: #{p.file?}"

GC.start

puts "File: #{p.file?}"

This generates:

$ ruby scratch.rb
File: true
File: false

This PR fixes the behavior by forcing a reference to the Tempfile object in the top level ProcessSpawn instance instead of expecting the Pathname @log instance variable to hold its reference.

@schneems schneems force-pushed the schneems/fix-gc-bug branch from f735f62 to 8614909 Compare July 20, 2020 19:33
A `Tempfile` works by defining a "finalizer" this is code that gets run when the object is cleaned up via GC. Here's a link:

https://github.com/ruby/ruby/blob/935d0b3d05dfc8b30bba505792129bf0e33ebe3b/lib/tempfile.rb#L132

As long as another object has a reference to a Tempfile then it will not be deleted from disk. In this scenario we're passing it to a `Pathname` object and it does not retain a reference to the passed in argument. You can reproduce this bug/behavior outside of Rundoc:

```
require 'pathname'
require 'tempfile'

def make_pathname(value = http://Tempfile.new("log"))
  http://Pathname.new(value)
end

p = make_pathname
FileUtils.touch(p)

puts "File: #{p.file?}"

GC.start

puts "File: #{p.file?}"
```

This generates:

```
$ ruby scratch.rb
File: true
File: false
```

This PR fixes the behavior by forcing a reference to the Tempfile object in the top level ProcessSpawn instance instead of expecting the Pathname `@log` instance variable to hold its reference.
@schneems schneems force-pushed the schneems/fix-gc-bug branch from 8614909 to 954ac24 Compare July 20, 2020 19:35
@schneems schneems merged commit 846a985 into master Jul 20, 2020
@chrisseaton
Copy link

What's with the http:// appearing in your code sample? That's not valid Ruby is it? It runs if I remove it.

@schneems
Copy link
Member Author

Thats from me copying it from twitter which thinks Pathname.new is a link with new as the TLD. I updated it. You need to run it as a file. It runs, but the GC.start deletes the file, which happens randomly in the code and has taken my month figure out with a few hours of active debugging.

schneems added a commit to heroku/java-getting-started that referenced this pull request Sep 3, 2020
I fixed the race condition here zombocom/rundoc#25
schneems added a commit to heroku/java-getting-started that referenced this pull request Sep 3, 2020
I fixed the race condition here zombocom/rundoc#25
LeePedersen pushed a commit to LeePedersen/technical-challenge-new-relic that referenced this pull request Feb 25, 2022
I fixed the race condition here zombocom/rundoc#25
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.

2 participants