Race condition in AsyncFileImpl writesOutstanding

[purplefox]

If an AsyncFileImpl is written to from a thread different to the context that created then the long counter writesOutstanding can get into an invalid state. This is because the counter is updated from both the caller's thread and also on the context when a write is complete.

Fix is to use an AtomicLong for writesOutstanding:

https://gist.github.com/purplefox/723fa69a1330fb4f4f74d4a5dd5e6658

[vietj]

a couple of questions:

• can you tell the unsynchronized write that happens ? is it the handler passed to the doWrite method that seems unsynchronized when called back
• is the atomicity of the long necessary ?
• could it be fixed using synchronized instead or setting the field as volatile ?

[vietj]

          context.runOnContext((v) -> {
            writesOutstanding -= buff.limit();
            handler.handle(Future.succeededFuture());
          });

[purplefox]

I think the issue is that writeOutstanding can be increased here:

https://github.com/eclipse/vert.x/blob/master/src/main/java/io/vertx/core/file/impl/AsyncFileImpl.java#L375

At the same time it is decreased here:

https://github.com/eclipse/vert.x/blob/master/src/main/java/io/vertx/core/file/impl/AsyncFileImpl.java#L395

If write is called from a different thread to the context thread.

I tried using synchronized to fix initially but this resulted in a deadlock. Volatile can't be used as volatile adds aren't atomic.

[vietj]

can you provide a reproducer ?

[purplefox]

Sure, will try and put one together :)

[purplefox]

Here's a reproducer that can run inside vertx-core FileSystemTest:

https://gist.github.com/purplefox/6de14775d331395ceeb01168367715f4

It needs to be run with many iterations to reproduce (hence the @repeat)

[purplefox]

If you run the test enough times you will find it hangs eventually, this is because the close() method never completes. It doesn't complete because it is waiting for an invalid value of outstanding writes.

[vietj]

makes sense thank!

[vietj]

I pushed a branch with a fix, I used synchronized because that's the class synchronization pattern and also it was necessary to read the closeDeferred field that is set in closeInternal using synchronization. The synchronized block does not propagate to handlers to avoid deadlocks.

[vietj]

thanks for the report, the patch and the reproducer!