Using new methods lockForUpdate and lockTimeout an instance of
raw_fd_ostream can be put into mode in which it locks underlying file
before each write and unlocks it immediately after. This is convenience
method, it allows transparent operations on log files in parallel builds,
when several processes write into the same file.
Details
Details
- Reviewers
ruiu labath csigg jhenderson dblaikie
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
While I don't really feel qualified to set the direction here, I gotta say that this patch (and the built-in timeout in particular) looks worrying to me.
Overall I feel that this sort of automatic locking will very rarely be the "right tool for the job". I think your follow-up patch sort of demonstrates that, because you've needed to format the output to a temporary string stream in order for it do what you need. I think it might be more reasonable to just leave this out of the raw_ostream class, and do a manual lockFile + write + unlockFile combo where needed. In fact, if we do that, I'm wondering if locking is really needed, as a write to a O_APPEND file is more-or-less guaranteed to be atomic (I think the only problem comes from signals, and I'm not sure if those really happen on local files):
O_APPEND
The file is opened in append mode. Before each write(2), the file offset is positioned
at the end of the file, as if with lseek(2). The modification of the file offset and
the write operation are performed as a single atomic step.I believe a similar effect on windows can be achieved with FILE_APPEND_DATA and/or writing to offset 0xffffffffffffffff
FILE_APPEND_DATA
For a file object, the right to append data to the file. (For local files, write operations will not overwrite existing data if this flag is specified without FILE_WRITE_DATA.)
WriteFile:
To write to the end of file, specify both the Offset and OffsetHigh members of the OVERLAPPED structure as 0xFFFFFFFF. This is functionally equivalent to previously calling the CreateFile function to open hFile using FILE_APPEND_DATA access.Comment Actions
Of course, this is a matter of convenience. Automatic locking, as implemented in this patch is not flexible enough, it must be improved. But manual operations might be error-prone.
In fact, if we do that, I'm wondering if locking is really needed, as a write to a O_APPEND file is more-or-less guaranteed to be atomic (I think the only problem comes from signals, and I'm not sure if those really happen on local files):
O_APPEND The file is opened in append mode. Before each write(2), the file offset is positioned at the end of the file, as if with lseek(2). The modification of the file offset and the write operation are performed as a single atomic step.
Here is a discussion about atomicity of append operation: https://stackoverflow.com/questions/1154446/is-file-append-atomic-in-unix. In short, actually atomicity of seek+write is provided only for data of small size. This statement agree with our experience. Initially we have implementation that relied on atomicity of append operation. Log files were regularly spoiled. Using file locks solved this problem.
Comment Actions
Well.. the thing is, I am not sure automatic locking *can* be improved to be useful. I find this situation very reminiscent of various "thread-safe" containers. While in theory we could have an std::thread_safe_map, I think there's a (good) reason that the standard library does not provide it -- it usually is not that useful, because one needs to have more control over the scope of the "critical sections". A typical use case for a thread-safe map is to compute something once and then cache it, but there one needs the whole test-and-set block to be atomic, not just individual operations.
I think this is fairly similar to what happens with raw_ostream, where one typically would wants to have locking granularity be different from what happens to fall out of the stream implementation. And in that case, I am not sure the implementation needs to be inside the stream class. Maybe an RAII object is in order ?
{
stream_locker Lock(OS); // not sure what to do about error handling
OS << whatever;
} // Lock destructor calls OS.flush(), and unlocks ?In fact, if we do that, I'm wondering if locking is really needed, as a write to a O_APPEND file is more-or-less guaranteed to be atomic (I think the only problem comes from signals, and I'm not sure if those really happen on local files):
O_APPEND The file is opened in append mode. Before each write(2), the file offset is positioned at the end of the file, as if with lseek(2). The modification of the file offset and the write operation are performed as a single atomic step.Here is a discussion about atomicity of append operation: https://stackoverflow.com/questions/1154446/is-file-append-atomic-in-unix. In short, actually atomicity of seek+write is provided only for data of small size. This statement agree with our experience. Initially we have implementation that relied on atomicity of append operation. Log files were regularly spoiled. Using file locks solved this problem.
Yeah, I had a feeling PIPE_BUF would come into play sooner or later. If that is not enough for your use case, then yes, I guess we will need to have some sort of locking primitives.
clang-format not found in user's PATH; not linting file.