Project

General

Profile

Actions

Bug #18077

closed

Marshal.dump(closed_io) raises IOError instead of TypeError

Added by larskanis (Lars Kanis) 2 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 3.1.0dev (2021-08-16T08:00:19Z master a8714b83c4) [x86_64-linux]
[ruby-core:104927]

Description

Marshal.dump is expected to raise a TypeError for unmarshallable objects. But closed streams raise an IOError:

$ ruby -e "io=IO.pipe.first; io.close; Marshal.dump(io)"
-e:1:in `internal_encoding': closed stream (IOError)
    from -e:1:in `dump'
    from -e:1:in `<main>'

This issue is present in all current ruby versions.

Updated by nobu (Nobuyoshi Nakada) 2 months ago

One concern for that PR, probably may not be a matter, is closed IOs will no longer raise IOError on other Encoding operations too.

$ ./ruby -v -e 'p Encoding.compatible?(Encoding::US_ASCII, File.open(IO::NULL).tap(&:close))'
ruby 3.1.0dev (2021-08-16T08:00:19Z master a8714b83c4) [x86_64-darwin19]
-e:1:in `internal_encoding': closed stream (IOError)
    from -e:1:in `compatible?'
    from -e:1:in `<main>'
$ ./ruby -v -e 'p Encoding.compatible?(Encoding::US_ASCII, File.open(IO::NULL).tap(&:close))'
ruby 3.1.0dev (2021-08-16T12:33:34Z master 9dd58a421b) [x86_64-darwin19]
last_commit=Fix Marshal.dump(closed_io) to raise TypeError
nil

Updated by larskanis (Lars Kanis) 2 months ago

https://github.com/ruby/ruby/pull/4749 is another fix without the above side effect. Is it OK?

Updated by Eregon (Benoit Daloze) 2 months ago

Why does IO#internal_encoding (and external_encoding) raise if the IO is closed?
They could just return the encoding, it's still stored in in the IO instance, and we are not trying to access the fd, right?

JRuby 9.2.17.0 does not raise, and I think it is the better behavior here.

Updated by mame (Yusuke Endoh) 2 months ago

I don't have any strong opinion but Eregon (Benoit Daloze)'s approach looks a bit better.

Updated by larskanis (Lars Kanis) 2 months ago

Eregon (Benoit Daloze) This was my first though as well, but the current behavior is already defined in ruby-spec.

I would prefer the behavioral change of IO#(in|ex)ternal_encoding being usable on closed IOs and prepared a PR: https://github.com/ruby/ruby/pull/4758

Updated by nobu (Nobuyoshi Nakada) 2 months ago

Since this is a bug report, I think that the old example in rubyspec should be removed.

Updated by Eregon (Benoit Daloze) 2 months ago

larskanis (Lars Kanis) wrote in #note-5:

Eregon (Benoit Daloze) This was my first though as well, but the current behavior is already defined in ruby-spec.

We can change it in ruby/spec (adding version guards and adding the new expectation).
And in fact I would be glad if the adapted specs would test the more sensible behavior.
ruby/spec often tests whatever the behavior of CRuby is (for best compatibility with other Rubies), it might not always make sense.

Updated by Eregon (Benoit Daloze) 2 months ago

I should have read the PR first.
Yes, removing the old example and adding the new one with a version guard is perfect here :)
That allows other implementations & older versions to potentially use the new behavior already, and there is probably very little to no code relying on the previous exception.

Actions #10

Updated by Anonymous 2 months ago

  • Status changed from Open to Closed

Applied in changeset git|6594623f623a0982da62cc105094da0701d499da.


Fix Marshal.dump(closed_io) to raise TypeError and allow encoding on closed IO

Mashalling a closed IO object raised "closed stream (IOError)" before instead of TypeError.
This changes IO#(in|ex)ternal_encoding to still return the encoding even if the underlying FD is closed.

Fixes bug #18077

Actions

Also available in: Atom PDF