As discovered in #3139, io.BufferedWriter mutates the size of its
internal bytearray object. Consequently, invocations of write() from
multiple threads can produce exceptions when one thread gets a buffer to
the bytearray while the other thread tries to resize it. This especially
affects calling print() from multiple threads.

A solution is to use a fixed-size (preallocated) bytearray object.
Another solution is to get rid of the bytearray approach and replace it
with a growing list of immutable bytes objects.

Here is the test script provided by Amaury:

import sys, io, threading
stdout2 = io.open(sys.stdout.fileno(), mode="w")
def f(i):
    for i in range(10):
        stdout2.write(unicode((x, i)) + '\n')
for x in range(10):
    t = threading.Thread(target=f, args=(x,))
    t.start()

(with py3k, replace "stdout2.write" with a simple "print")

Here is a sample implementation (using a fixed-size bytearray) with test.
Amaury, would you like to take a look?

I'm a bit puzzled by the shady BufferedRandom semantics: though the
tests do pass, it's hard to say why e.g. BufferedRandom.tell() was
implemented like that.

It seems that as with the quadratic binary buffered reading, the best
solution is the list of bytes which should be especially helped by your
bytes joining optimization.

Selon Benjamin Peterson <report@bugs.python.org>:
>
> Benjamin Peterson <musiccomposition@gmail.com> added the comment:
>
> It seems that as with the quadratic binary buffered reading, the best
> solution is the list of bytes which should be especially helped by your
> bytes joining optimization.

I don't really know. The logic is quite different (and harder to get right) than
in BufferedReader. I might try to write a list-of-bytes version and do some
timings, but it's not trivial... I should first compare against the current
version.

Also, an advantage of using a fixed-size bytearray is that it more closely
mimicks what will have to be written for an efficient C implementation.

On Thu, Jul 31, 2008 at 9:36 AM, Antoine Pitrou <report@bugs.python.org> wrote:
>
> I don't really know. The logic is quite different (and harder to get right) than
> in BufferedReader. I might try to write a list-of-bytes version and do some
> timings, but it's not trivial... I should first compare against the current
> version.
>
> Also, an advantage of using a fixed-size bytearray is that it more closely
> mimicks what will have to be written for an efficient C implementation.

You are correct that this should probably be rewritten in C at some
point, so let's go with this.

I don't think the proposed patch (file11012) makes it thread-safe, and I
believe you cannot get it thread-safe without using thread synchronization.

For example, if two threads simultaneously determine that there is still
space left (len(b) <= free), they might both write to self._write_buf
and self._write_end, and the last writer would win. (In case it isn't
clear how this might happen: thread 1 exhausts its Py_Ticker just after
checking the size, then thread 2 runs a full .write(), then thread 1
continues filling the buffer)

Hi,

Selon "Martin v. Löwis" <report@bugs.python.org>:
>
> I don't think the proposed patch (file11012) makes it thread-safe, and I
> believe you cannot get it thread-safe without using thread synchronization.

I should have precised that in the context of this issue, "thread-safe" does not
mean "produces perfectly correct output" but simply "does not raise exceptions
when using the same buffered object from two different threads". The former
would be preferable but is not required, IMHO, for a buffered IO library; the
latter is much more critical because as Amaury points out, you otherwise get
exceptions when printing e.g. debut output from multiple threads.

Attaching a new patch with better performance characteristics than my
previous one, and the non-blocking test rewritten in a sane way.

Some timeit runs:

-s "import io; f=io.open('/dev/null', 'wb'); s=b'a'*1" "for i in xrange(100): f.write(s)"

without patch: 533 usec per loop
with patch: 724 usec per loop
with builtin open(): 59.6 usec per loop

-s "import io; f=io.open('/dev/null', 'wb'); s=b'a'*100" "for i in xrange(100): f.write(s)"

without patch: 563 usec per loop
with patch: 768 usec per loop
with builtin open(): 67.8 usec per loop

-s "import io; f=io.open('/dev/null', 'wb'); s=b'a'*10000" "for i in xrange(100): f.write(s)"

without patch: 1.35 msec per loop
with patch: 1.34 msec per loop
with builtin open(): 194 usec per loop

-s "import io; f=io.open('/dev/null', 'wb'); s=b'a'*100000" "for i in xrange(100): f.write(s)"

without patch: 4.92 msec per loop
with patch: 1.34 msec per loop
with builtin open(): 199 usec per loop

-s "import io; f=io.open('/dev/null', 'wb'); s=b'a'*1000000" "for i in xrange(100): f.write(s)"

without patch: 173 msec per loop
with patch: 1.35 msec per loop
with builtin open(): 194 usec per loop

> I should have precised that in the context of this issue, "thread-safe" does not
> mean "produces perfectly correct output" but simply "does not raise exceptions
> when using the same buffered object from two different threads".

In that case, I'm -1 for this patch. Raising exceptions is much more
preferable to silently losing data, or writing garbage.

> The former would be preferable but is not required, IMHO, for a buffered IO library; the
> latter is much more critical because as Amaury points out, you otherwise get
> exceptions when printing e.g. debut output from multiple threads.

And that's a good thing, when using a library that is not thread-safe.

Either the library provides thread-safety, or it doesn't. If it doesn't,
it's the application's responsibility to not use the library from
multiple threads, or protect all access with appropriate
synchronization. Now that print is a function, it's easy to implement
a version of it that synchronizes all prints.

With the status quo, people have at least a chance of learning that the
library is not thread-safe. If the shallow problems are resolved, people
will cry FOUL loudly when they learn about the deep problems.

Le vendredi 01 août 2008 à 21:51 +0000, Martin v. Löwis a écrit :
> With the status quo, people have at least a chance of learning that the
> library is not thread-safe. If the shallow problems are resolved, people
> will cry FOUL loudly when they learn about the deep problems.

But the current implementation can also silently produce garbage without
raising an exception. It's only if the context switch happens at a
certain precise point that an exception is raised. If the internal
buffer is mutated without resizing of the backing memory area (or
without any buffer being currently held), there is no exception and
still the behaviour is incorrect.

> Now that print is a function, it's easy to implement
> a version of it that synchronizes all prints.

Well, if that resolution is prefered, I think it should be integrated to
the builtin print function, rather than forcing users to monkeypatch it
(but a debugging facility directly calling sys.stdout.write or
equivalent will not be helped).

> Well, if that resolution is prefered, I think it should be integrated to
> the builtin print function, rather than forcing users to monkeypatch it
> (but a debugging facility directly calling sys.stdout.write or
> equivalent will not be helped).

True. I think it is the stream's write method that must be
synchronized, e.g. by renaming the current write function to
_write_locked, and adding a write function that obtains a per-file
lock, and calls write_locked. Other methods accessing the buffer
need to get synchronized with the same lock as well.

This is how it's done in about any stdio implementation I ever looked at.

Here is a new patch which simply wraps the current BufferedWriter
methods with a lock. It has a test case, and Amaury's example works fine
too.

Martin, do you think it's fine?

(as for BufferedReader, I don't see the use cases for multithreaded reading)

The patch looks fine (as far as it goes).

I do think the same should be done to the reader: IO libraries typically
provide a promise that concurrent threads can read, and will get the
complete stream in an overlapped manner (i.e. each input byte goes to
exactly one thread - no input byte gets lost, and no input byte is
delivered to multiple threads). I don't think this is currently the
case: two threads reading simultaneously may very well read the same
bytes twice, and then, subsequently, skip bytes (i.e. when both
increment _read_pos, but both see the original value of pos)

Both BufferedReader and BufferedWriter are now fixed in r65686.
Perhaps someone wants to open a separate issue for TextIOWrapper...