On 28 August 2017 at 13:44, Evgeny Kotkov <evgeny.kot...@visualsvn.com> wrote: > Hi everyone, > > Currently, appending data to a file opened with APR_FOPEN_APPEND > that has been locked by apr_file_lock() will cause a deadlock on Windows. > > [See PR50058, https://bz.apache.org/bugzilla/show_bug.cgi?id=50058] > > This issue happens because atomic O_APPEND-style appends on Windows > are implemented using file locks. An append happens while holding the > file lock acquired with LockFile(), which is required to avoid a race > condition between seeking to the end of file and writing data. The > race is possible when multiple threads or processes are appending > data to the same file. > > This approach causes a deadlock if the file has been previously locked > with apr_file_lock(). (Note that it's perfectly legit to lock the file or > its portion and perform the append after that.) > > Apart from this, using file locks for file appends impacts their speed and > robustness. There is an overhead associated with locking the file, especially > if the file is not local. The robustness is affected, because other writes > to the same file may fail due to it being locked. Also, if a process is > terminated in the middle of the append operation, it might take some time > for the OS to release the file lock. During this time, the file would be > inaccessible to other processes. This may affect applications such as > httpd (with a multi-process MPM) that use APR_FOPEN_APPEND files for > logging. > > This patch series fixes the issue by switching to the documented way to > atomically append data with a single WriteFile() call. It requires passing > special OVERLAPPED.Offset and OffsetHigh values (0xFFFFFFFF). On the > ZwWriteFile() layer, this maps to a FILE_WRITE_TO_END_OF_FILE constant > that instructs the OS (the corresponding file system driver) to write data > to the file's end. > > Note that this approach is only used for files opened for synchronous I/O > because in this case the I/O Manager maintains the current file position. > Otherwise, the file offset returned or changed by the SetFilePointer() API is > not guaranteed to be valid and that could, for instance, break apr_file_seek() > calls after appending data. Sadly, if a file is opened for asynchronous I/O, > this call to WriteFile() doesn't update the OVERLAPPED.Offset member to > reflect the actual offset used when appending the data (which we could > then use to make seeking and other operations involving filePtr work). > Therefore, when appending to files opened for asynchronous I/O, we still > use the old LockFile + SetFilePointer + WriteFile approach. > > Additional details on this can be found in: > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365747 > https://msdn.microsoft.com/en-us/library/windows/hardware/ff567121 > > > The implementation is split into four dependent patches. The first three > patches are minor refactorings to the apr_file_write() function and lay the > necessary groundwork for the core change. The fourth patch is the core > change that fixes the issue in the described way and also includes the > tests. > > The log messages are included in the beginning of each patch file. > > 1-file-write-simplify-local-vars-v1.patch.txt Committed in r1806592.
> 2-file-write-simplify-fileptr-inc-v1.patch.txt Committed in r1806603. > 3-file-write-invert-if-condition-v1.patch.txt Committed in r1806604. > 4-fix-locked-append-deadlock-v1.patch.txt Committed in r1806608. Thanks! -- Ivan Zhakov
