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

IO.copy_stream keep pos of destination to beginning of file #3280

Closed
HoneyryderChuck opened this issue Sep 29, 2023 · 7 comments
Closed

IO.copy_stream keep pos of destination to beginning of file #3280

HoneyryderChuck opened this issue Sep 29, 2023 · 7 comments
Assignees
Labels

Comments

@HoneyryderChuck
Copy link
Contributor

This is the minimum repro I could make:

require "stringio"
require "tempfile"
a = StringIO.new
a << ("a"*100000)
b = Tempfile.new("test", encoding: Encoding::BINARY, mode: File::RDWR)
a.rewind
IO.copy_stream(a, b)
puts b.pos

Using ruby 3.2, the output is 100000, whereas in truffleruby, output is 0.

version was truffleruby 22.3.1, like ruby 3.0.3, GraalVM CE Native [x86_64-linux] (I use truffleruby for tests in docker, there is no v23 image available yet).

HoneyryderChuck added a commit to HoneyryderChuck/httpx that referenced this issue Sep 29, 2023
@eregon eregon added the bug label Oct 2, 2023
@eregon eregon self-assigned this Oct 2, 2023
@eregon eregon added this to the 24.0.0 Release (March 19, 2024) milestone Oct 2, 2023
@eregon
Copy link
Member

eregon commented Oct 2, 2023

Thank you for the report.
This bug is due to

elsif obj.respond_to? :to_path
path = Truffle::Type.coerce_to obj, String, :to_path
io = File.open path, mode

A Tempfile is not an IO or a File, it's a separate class using delegation:

irb(main):006:0> Tempfile.ancestors
=> [Tempfile, #<Class:0x00007f9b2de25e80>, Delegator, #<Module:0x00007f9b2de0a338>, BasicObject]

And so the coercion tries to_path which does exist on on Tempfile and Delegates to File#to_path, and therefore it creates another IO object.
I will check what's the actual logic in CRuby, maybe we shouldn't try to_path there or maybe there is a special case for Tempfile.

@eregon
Copy link
Member

eregon commented Oct 2, 2023

https://github.com/ruby/ruby/blob/2d6067dc7d02c5942da6636bba8b1629065aecf0/io.c#L13160 is the conversion logic in CRuby.
So it check if dst is ARGF or is not (an IO or a String or a #to_path), then dst_fptr = NULL.
And if that doesn't hold (Tempfile has to_path) then it tries rb_io_check_io which is #to_io first, then if not an IO tries FilePathValue (#to_path) and otherwise uses it as-is as an IO.

graalvmbot pushed a commit that referenced this issue Oct 2, 2023
@eregon
Copy link
Member

eregon commented Oct 2, 2023

Fix in #3282

@HoneyryderChuck
Copy link
Contributor Author

Thx for the prompt fix @eregon 🙏

Unrelated, but when can one expect docker truffleruby images targeting v23?

@eregon
Copy link
Member

eregon commented Oct 3, 2023

@HoneyryderChuck The Docker images for 23.0 and 23.1 are already available: https://github.com/graalvm/container/blob/master/truffleruby-community/README.md

@HoneyryderChuck
Copy link
Contributor Author

Oh, i was still using ghcr.io/graalvm/truffleruby:latest, didn't know about the change. will upate CI builds accoringly, thx!

HoneyryderChuck added a commit to HoneyryderChuck/httpx that referenced this issue Oct 3, 2023
eregon added a commit to ruby/spec that referenced this issue Oct 30, 2023
mtortonesi pushed a commit to mtortonesi/truffleruby that referenced this issue Nov 10, 2023
@eregon
Copy link
Member

eregon commented Dec 4, 2023

This fix will be in the 23.1.2 release which will be available on January 23, 2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants