[PATCH] D78897: [Support] raw_fd_ostream can lock file before write

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D78897#2007851 <https://reviews.llvm.org/D78897#2007851>, @sepavloff wrote:

> In D78897#2005085 <https://reviews.llvm.org/D78897#2005085>, @labath wrote:
>
> > 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.
>
>
> 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.


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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78897/new/

https://reviews.llvm.org/D78897