Don't hide ffi-inliner errors #82
Conversation
If ffi-inliner isn't installed the earlier lines of these methods will raise an error and the Archive::Stat module won't be installed, so Archive::Stat.ffi_libarchive_free_stat doens't exist and also raises an error during the ensure block, hiding the originally-raised error. Signed-off-by: Samuel Cochran <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
|
Re: ffi-libarchive/lib/ffi-libarchive/entry.rb Line 177 in dae72e2 I've been playing around — perhaps there is a way to have a minimal extension like this which can reach into File::Stat and avoid the need for ffi-inliner? https://github.com/sj26/fiddle-file-stat (I've done it in Fiddle, but the same should apply for FFI.) |
| begin | ||
| stat = Archive::Stat.ffi_libarchive_create_lstat(filename) | ||
| raise Error, "Copy stat failed: #{Archive::Stat.ffi_error}" if stat.null? | ||
|
|
||
| C.archive_entry_copy_stat(entry, stat) | ||
| ensure | ||
| Archive::Stat.ffi_libarchive_free_stat(stat) | ||
| C.archive_entry_copy_stat(entry, stat) | ||
| ensure | ||
| Archive::Stat.ffi_libarchive_free_stat(stat) | ||
| end |
There was a problem hiding this comment.
Maybe I'm wrong, but won't this still be the same? It'll still raise an error, the ensure will still fire, the free_stat will still fire, no?
[phil@rider ~]$ cat /tmp/test2.rb
#!/usr/bin/ruby
def test
puts "foo"
begin
raise 'test error'
ensure
raise 'ensure error'
end
end
test
[phil@rider ~]$ /tmp/test.rb
foo
/tmp/test.rb:8:in `ensure in test': ensure error (RuntimeError)
from /tmp/test.rb:8:in `test'
from /tmp/test.rb:11:in `<main>'
/tmp/test.rb:6:in `test': test error (RuntimeError)
from /tmp/test.rb:11:in `<main>'
[phil@rider ~]$ /tmp/test2.rb
foo
/tmp/test2.rb:9:in `ensure in test': ensure error (RuntimeError)
from /tmp/test2.rb:9:in `test'
from /tmp/test2.rb:13:in `<main>'
/tmp/test2.rb:7:in `test': test error (RuntimeError)
from /tmp/test2.rb:13:in `<main>'
I think you want to rescue the specific error you're looking for.
Or perhaps you want to not call ffi_libarchive_free_stat if stat is null?
There was a problem hiding this comment.
Ah, sorry, I didn't explain very well. The error comes from the require on line 179, while loading the file and running ffi-inliner. That is caught a another error raised by 181. Then the ensure block runs line 189 which has no hope of succeeding.
It's closer to:
def test
begin
raise "original error"
rescue
raise "had an error"
end
foo = true
ensure
unless foo
raise "unrelated error"
end
end
test/tmp/test.rb:10:in `ensure in test': unrelated error (RuntimeError)
from /tmp/test.rb:10:in `test'
from /tmp/test.rb:14:in `<main>'
/tmp/test.rb:5:in `rescue in test': had an error (RuntimeError)
from /tmp/test.rb:2:in `test'
from /tmp/test.rb:14:in `<main>'
/tmp/test.rb:3:in `test': original error (RuntimeError)
from /tmp/test.rb:14:in `<main>'
and this refactors to:
def test
begin
raise "original error"
rescue
raise "had an error"
end
begin
foo = true
ensure
unless foo
raise "unrelated error"
end
end
end
test/tmp/test.rb:5:in `rescue in test': had an error (RuntimeError)
from /tmp/test.rb:2:in `test'
from /tmp/test.rb:16:in `<main>'
/tmp/test.rb:3:in `test': original error (RuntimeError)
from /tmp/test.rb:16:in `<main>'
It prevents the ensure block coming into context unless the condition it is protecting against can exist, preventing a red herring during error diagnosis for the user.
If ffi-inliner isn't installed the earlier lines of these methods will raise an error and the Archive::Stat module won't be installed, so Archive::Stat.ffi_libarchive_free_stat doens't exist and also raises an error during the ensure block, hiding the originally-raised error.
Types of changes
Checklist: