Incorrect handling of EINTR in Bazaar

[Andrew Bennetts]

Martin (gzlist) wrote:
[...]
>   file.read
>     Bogus, seems high level block io (fread) can't be interrupted

I don't think this is right.  Even when 'file' is a pipe, e.g. when using
stdin/stdout?  I certainly see EINTR in this case.  Try this:

python -c 'import os, signal, sys; print os.getpid(); signal.signal(signal.SIGUSR1, lambda *args: None); print repr(sys.stdin.read(3))'

It will print its PID and block.  If I then do “kill -USR1 $THAT_PID” I get this
result:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
IOError: [Errno 4] Interrupted system call

>   file.write
>     Bogus, seems high level block io (fwrite) can't be interrupted

The man page (on my Ubuntu system) for fwrite is not clear, but it
explicitly talks about short writes (including 0-length writes), and I would
expect a signal interrupting a write could cause that with a valid
implementation of fwrite, and file_write in Objects/fileobject.c would
certainly turn that short write into an IOError.

>   file.flush
>     Bogus, seems high level block io (fflush) can't be interrupted

The man page for fflush says that any error from write(2) is possible, and
Google finds man pages from e.g. SunOS that explicitly list EINTR as a
possibility.  Those man pages don't say anything about whether it's safe to
retry the flush operation in that case, but I would expect it would be safe.
(I'd expect a platform's fflush implementation to be able to cope with short
writes just as well its fwrite implementation, but perhaps I'm too
optimistic?)

>   file.close
>     Bogus as first invalidates the internal handle so no point
> re-calling, see close_the_file in Objects/fileobject.c

My man page for close(2) lists the errors it can return, and includes:

       EINTR  The close() call was interrupted by a signal; see signal(7).

Perhaps not actually possible in practice for regular files or pipes?  It
does look like a bug in Python, it will leak the fd if EINTR occurs.

Anyway, you're right, we can't usefully catch EINTR from Python in this
case.  Well, actually, we could remember the fileno, and if EINTR occurs
then until_no_eintr(os.close, fileno), but that's probably not worth the
effort just to avoid leaking an fd in this corner case.

>   StringIO.StringIO.*
>     Bogus as will never throw EINTR

I'd say “harmless”, rather than “bogus” ;)

>   socket._fileobject.flush
>     Can't be safely re-called, as it uses socket._socketobject.sendall
> internally
>   socket._fileobject.close
>     Can't be safely re-called, as it uses socket._fileobject.flush and
> always calls socket._socketobject.close
>   socket._fileobject.write
>     Can't be safely re-called, as appends to an instance buffer
> unconditionally and may use socket._fileobject.flush
>   socket._fileobject.read
>     Can't be safely re-called, because it uses a local buffer and
> multiple recv calls

Ouch!

[...]
> Clearly there shouldn't be any servers left around by the time the
> world is being dismantled, but the long-standing leaks mean there are.
> Basically, SmartTCPClientMedium.disconnect is called from
> SmartClientStreamMedium.__del__ when the module is in process of being
> cleaned up so osutils is gone, and an exception trying to call
> osutils.until_no_eintr results. Resolving issues like this with
> __del__ is easy, but pointless as we've already seen that wrapping
> close calls does nothing of use.

Well, chances are we'll need to fix or remove that __del__ eventually
anyway.  It's likely that some other module-global will be required at
some point (if not in today's implementation, quite possibly in next
month's...).  So this isn't much of an argument for or against anything
except being more cautious with our code in __del__, or removing those
__del__ methods entirely (in favour of weakref callbacks, perhaps?).

-Andrew.