Re: IPC::Run accepts bug reports

On Tue, Jun 18, 2024 at 08:07:27PM -0700, Andres Freund wrote:
> > > > 1) Sometimes hangs hard on windows if started processes have not been shut
> > > >    down before script exits.

> It reliably reproduces if I comment out
> the lines below
>   # explicitly shut down psql instances gracefully - to avoid hangs
>   # or worse on windows
> in 021_row_visibility.pl
> 
> The logfile ends in
> Warning: unable to close filehandle GEN25 properly: Bad file descriptor during global destruction.
> Warning: unable to close filehandle GEN20 properly: Bad file descriptor during global destruction.
> 
> 
> Even if I cancel the test, I can't rerun it because due to a leftover psql
> a) a new temp install can't be made (could be solved by rm -rf)
> b) the test's logfile can't be removed (couldn't even rename the directory)
> 
> The psql instance needs to be found and terminated first.

Thanks for that recipe.  I've put that in my queue to fix.

On Tue, Jun 18, 2024 at 12:00:13PM -0700, Andres Freund wrote:
> On 2024-06-18 10:10:17 -0700, Noah Misch wrote:
> > On Mon, Jun 17, 2024 at 11:11:17AM -0700, Andres Freund wrote:
> > > 2) If a subprocess dies in an inopportune moment, IPC::Run dies with "ack
> > >    Broken pipe:" (in _do_filters()). There's plenty reports of this on the
> > >    list, and I've hit this several times personally. It seems to be timing
> > >    dependent, I've encountered it after seemingly irrelevant ordering changes.
> > > 
> > >    I suspect I could create a reproducer with a bit of time.
> > 
> > I've seen that one.  If the harness has data to write to a child, the child
> > exiting before the write is one way to reach that.  Perhaps before exec(),
> > IPC::Run should do a non-blocking write from each pending IO.  That way, small
> > writes never experience the timing-dependent behavior.
> 
> I think the question is rather, why is ipc run choosing to die in this
> situation and can that be fixed?

With default signal handling, the process would die to SIGPIPE.  Since
PostgreSQL::Test ignores SIGPIPE, this happens instead.  The IPC::Run source
tree has no discussion of ignoring SIGPIPE, so I bet it didn't get a conscious
decision.  Perhaps it can do better.