Mailing List Archive

release on hold: GH #21187
Porters,

https://github.com/Perl/perl5/issues/21187 has been reported as "close() seems to have changed behavior unexpectedly". My initial reading suggests this is a bug, and I'm not keen to ship with close() broken. But I didn't make the change under discussion, and I'm not sure what we might expect.

Thoughts and/or fixes?

--
rjbs
Re: release on hold: GH #21187 [ In reply to ]
On Sat, Jul 1, 2023, at 10:59, Ricardo Signes wrote:
> Thoughts and/or fixes?

On IRC, Bram notes that this seems to have come up a few times and been reported as expected:

• See https://github.com/Perl/perl5/pull/20103 and https://github.com/Perl/perl5/issues/20060
• and a similar report about it https://github.com/Perl/perl5/issues/20376 )
• I suppose https://github.com/Perl/perl5/blob/blead/pod/perldelta.pod#readline-no-longer-clears-the-stream-error-and-eof-flags could/should be updated to make it more clear on how it affects `close()`
• Technically it's in there: `Since the error flag is no longer cleared calling close() on the stream may fail
--
rjbs
Re: release on hold: GH #21187 [ In reply to ]
On Sat, 1 Jul 2023 at 16:10, Ricardo Signes <perl.p5p@rjbs.manxome.org> wrote:
> I suppose https://github.com/Perl/perl5/blob/blead/pod/perldelta.pod#readline-no-longer-clears-the-stream-error-and-eof-flags could/should be updated to make it more clear on how it affects `close()`
> Technically it's in there: `Since the error flag is no longer cleared calling close() on the stream may fail
>

Yes. I hadn't spotted that before. But as I just noted on #21187, I
still think the change in behaviour of close() is wrong. If possible
we should fix the wrongness, rather than documenting it in perldelta.
Re: release on hold: GH #21187 [ In reply to ]
Can anyone comment on:

1. the difficulty of fixing this
2. whether anybody things the new behavior is not merely "tolerable" but "right"
?

On Sat, Jul 1, 2023, at 13:29, Steve Hay wrote:
> On Sat, 1 Jul 2023 at 16:10, Ricardo Signes <perl.p5p@rjbs.manxome.org> wrote:
> > I suppose https://github.com/Perl/perl5/blob/blead/pod/perldelta.pod#readline-no-longer-clears-the-stream-error-and-eof-flags could/should be updated to make it more clear on how it affects `close()`
> > Technically it's in there: `Since the error flag is no longer cleared calling close() on the stream may fail
> >
>
> Yes. I hadn't spotted that before. But as I just noted on #21187, I
> still think the change in behaviour of close() is wrong. If possible
> we should fix the wrongness, rather than documenting it in perldelta.
>
Re: release on hold: GH #21187 [ In reply to ]
On 1/07/2023 21:53, Ricardo Signes wrote:
> Can anyone comment on:
I think the best one to comment on it might be @tonycoz
> 1. the difficulty of fixing this
The insights I can remember (I haven't worked on this):

*Before*:

* `<>` cleared the error flag, the following `close()` would only
return when the system `close()` call failed
* `print()` does not clear the error flag and a `close()` after a
`print()` (with error) would fail (I'm guessing always but haven't
verified!)

*Now*:

* `<>` does not clear the error flag; `close()` following a failed
`<>` will error

The question is then: if a fix is desired what should it be?

* `<>` clearing the error flag again? (and leaving no trace of the
error?)
* `close()` ignoring errors raised by `<>` but do error on errors
raised by `print()`? (I suppose that was the old behavior)
* ...?


> 2. whether anybody things the new behavior is not merely "tolerable"
> but "right"
Personally I have no strong opinion on whether it's right or wrong.

Two things that do come in mind:

* it's more consistent with `print()`
* perldoc -f close: "Closes the file or pipe associated with the
filehandle, flushes the IO buffers, and closes the system file
descriptor. Returns true if those operations succeed and if *no
error was reported by any PerlIO layer*.*" --> *It all depends on
what that last part ("no error was reported by any PerlIO layer")
means.


Bram
Re: release on hold: GH #21187 [ In reply to ]
On Sat, Jul 1, 2023 at 5:00?PM Ricardo Signes <perl.p5p@rjbs.manxome.org>
wrote:

> Porters,
>
> https://github.com/Perl/perl5/issues/21187 has been reported as "close()
> seems to have changed behavior unexpectedly". My initial reading suggests
> this is a bug, and I'm not keen to ship with close() broken. But I didn't
> make the change under discussion, and I'm not sure what we might expect.
>
> Thoughts and/or fixes?
>

I strongly suspect the recently observed HPUX (and AIX) issues with stdio
are also related to this.

Leon
Re: release on hold: GH #21187 [ In reply to ]
On Sat, 1 Jul 2023, 23:39 Leon Timmermans, <fawaka@gmail.com> wrote:

> On Sat, Jul 1, 2023 at 5:00?PM Ricardo Signes <perl.p5p@rjbs.manxome.org>
> wrote:
>
>> Porters,
>>
>> https://github.com/Perl/perl5/issues/21187 has been reported as "close()
>> seems to have changed behavior unexpectedly". My initial reading suggests
>> this is a bug, and I'm not keen to ship with close() broken. But I didn't
>> make the change under discussion, and I'm not sure what we might expect.
>>
>> Thoughts and/or fixes?
>>
>
> I strongly suspect the recently observed HPUX (and AIX) issues with stdio
> are also related to this.
>

I agree.

Yves

>
Re: release on hold: GH #21187 [ In reply to ]
On Sat, 1 Jul 2023 at 21:44, Bram <p5p@perl.wizbit.be> wrote:
>
> On 1/07/2023 21:53, Ricardo Signes wrote:
> > Can anyone comment on:
> I think the best one to comment on it might be @tonycoz
> > 1. the difficulty of fixing this
> The insights I can remember (I haven't worked on this):
>
> *Before*:
>
> * `<>` cleared the error flag, the following `close()` would only
> return when the system `close()` call failed
> * `print()` does not clear the error flag and a `close()` after a
> `print()` (with error) would fail (I'm guessing always but haven't
> verified!)
>

close() after a print() with error does not always fail - see my
example on the ticket of print() on a read-only filehandle, where
close() still returns success. That behaviour hasn't changed.
(However, in some cases close() after a failed print() does fail - as
in your example of a print() to a device with no space.)

> *Now*:
>
> * `<>` does not clear the error flag; `close()` following a failed
> `<>` will error
>
> The question is then: if a fix is desired what should it be?
>
> * `<>` clearing the error flag again? (and leaving no trace of the
> error?)
> * `close()` ignoring errors raised by `<>` but do error on errors
> raised by `print()`? (I suppose that was the old behavior)
> * ...?
>

My preference would be what the system level operations do: namely,
each one (read, write, close) simply returns success or failure
according to whether it has succeeded or failed. If an earlier
operation sets an error state, then that is fine. The state can remain
and be examined later (if it hasn't been superseded by another error,
or explicitly cleared by the user), but an error on a previous
operation should not cause the current operation to return failure
when it has actually succeeded itself.

In other words, the error set by <> should not be cleared; but neither
should it cause close() to return failure.

However, I have no idea how difficult or even feasible that may be for
the perl operations, and there is always that ambiguous comment
(mentioned below) in the docs about errors reported by PerlIO layers
causing close to return failure. Does that mean that previous
operations are intended to interfere with close()'s return value?!

>
> > 2. whether anybody things the new behavior is not merely "tolerable"
> > but "right"
> Personally I have no strong opinion on whether it's right or wrong.
>
> Two things that do come in mind:
>
> * it's more consistent with `print()`

Well, it depends which print() case you consider. It's consistent with
the print on a device with no space, but not consistent with print on
a read-only filehandle ;-)

> * perldoc -f close: "Closes the file or pipe associated with the
> filehandle, flushes the IO buffers, and closes the system file
> descriptor. Returns true if those operations succeed and if *no
> error was reported by any PerlIO layer*.*" --> *It all depends on
> what that last part ("no error was reported by any PerlIO layer")
> means.
>
>
> Bram
>
Re: release on hold: GH #21187 [ In reply to ]
>>> Can anyone comment on:
>>>
>> I think the best one to comment on it might be @tonycoz
>>
>>> 1. the difficulty of fixing this
>>>
>> The insights I can remember (I haven't worked on this):
>>
>> *Before*:
>>
>> * `<>` cleared the error flag, the following `close()` would only
>> return when the system `close()` call failed
>> * `print()` does not clear the error flag and a `close()` after a
>> `print()` (with error) would fail (I'm guessing always but haven't
>> verified!
> close() after a print() with error does not always fail - see my
> example on the ticket of print() on a read-only filehandle, where
> close() still returns success. That behaviour hasn't changed.
> (However, in some cases close() after a failed print() does fail - as
> in your example of a print() to a device with no space.)
>
I believe that is a very specific case which Perl doesn't handle
correctly and is the inconsitent one..

>> *Now*:
>>
>> * `<>` does not clear the error flag; `close()` following a failed
>> `<>` will error
>>
>> The question is then: if a fix is desired what should it be?
>>
>> * `<>` clearing the error flag again? (and leaving no trace of the
>> error?)
>> * `close()` ignoring errors raised by `<>` but do error on errors
>> raised by `print()`? (I suppose that was the old behavior)
>> * ...?
>>
>>
>
> My preference would be what the system level operations do: namely,
> each one (read, write, close) simply returns success or failure
> according to whether it has succeeded or failed.
I'm afraid that ship has sailed long ago:
https://github.com/Perl/perl5/commit/e199e3bef188fd5eadcca420af186241e2773db1
```
commit e199e3bef188fd5eadcca420af186241e2773db1
Author: Rafael Garcia-Suarez <rgarciasuarez@gmail.com>
Date: Thu Mar 10 17:42:54 2005 +0000

Make the return value of close() depend not only on the success of the
close itself, but also on whether the output stream had a previous
error. From Jim Meyering <jim@meyering.net>, via Debian.

p4raw-id: //depot/perl@24022
```

(And changing that now would (also) be a breaking change)
Re: release on hold: GH #21187 [ In reply to ]
On Sat, 1 Jul 2023 at 23:08, Bram <p5p@perl.wizbit.be> wrote:
> > My preference would be what the system level operations do: namely,
> > each one (read, write, close) simply returns success or failure
> > according to whether it has succeeded or failed.
> I'm afraid that ship has sailed long ago:
> https://github.com/Perl/perl5/commit/e199e3bef188fd5eadcca420af186241e2773db1
> ```
> commit e199e3bef188fd5eadcca420af186241e2773db1
> Author: Rafael Garcia-Suarez <rgarciasuarez@gmail.com>
> Date: Thu Mar 10 17:42:54 2005 +0000
>
> Make the return value of close() depend not only on the success of the
> close itself, but also on whether the output stream had a previous
> error. From Jim Meyering <jim@meyering.net>, via Debian.
>
> p4raw-id: //depot/perl@24022
> ```
>
> (And changing that now would (also) be a breaking change)
>

Nice piece of code archaeology! Yes, my preference would indeed be
breaking a change of some standing :-( I wasn't aware of that
behaviour, which seems to be stated much more clearly in that commit
message than in the rather ambiguous perldoc entry for close().
(Perhaps that wording should be improved?)

In light of this it is, as you say, the close() after a print() on a
read-only filehandle case which is wrong, which isn't a new failure. I
will raise a new GH issue for that, but it's neither new nor urgent.

So should we close off #21187 and go ahead with the 5.38.0 release? Or
are there still problems with HPUX/AIX mentioned by Leon/Yves?
Re: release on hold: GH #21187 [ In reply to ]
On Sat, Jul 1, 2023 at 11:53?PM Steve Hay via perl5-porters <
perl5-porters@perl.org> wrote:

> My preference would be what the system level operations do: namely,
> each one (read, write, close) simply returns success or failure
> according to whether it has succeeded or failed. If an earlier
> operation sets an error state, then that is fine. The state can remain
> and be examined later (if it hasn't been superseded by another error,
> or explicitly cleared by the user), but an error on a previous
> operation should not cause the current operation to return failure
> when it has actually succeeded itself.
>
> In other words, the error set by <> should not be cleared; but neither
> should it cause close() to return failure.
>
> However, I have no idea how difficult or even feasible that may be for
> the perl operations, and there is always that ambiguous comment
> (mentioned below) in the docs about errors reported by PerlIO layers
> causing close to return failure. Does that mean that previous
> operations are intended to interfere with close()'s return value?!
>

Except buffered IO is inherently not a system level operation; it's
ultimately a case of "all abstractions leak somewhere". I don't really have
the answers here, it's all compromises.

Leon
Re: release on hold: GH #21187 [ In reply to ]
On Sat, Jul 1, 2023, at 18:34, Steve Hay wrote:
> So should we close off #21187 and go ahead with the 5.38.0 release? Or
> are there still problems with HPUX/AIX mentioned by Leon/Yves?

I'm going to plan on releasing tomorrow morning unless there is an extremely compelling argument to not, in the interim.

--
rjbs