[Lldb-commits] [PATCH] D77295: [lldb/Core] Fix a race in the Communication class

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: lldb/source/Core/Communication.cpp:318
   bool done = false;
+  bool disconnect = false;
   while (!done && comm->m_read_thread_enabled) {
----------------
clayborg wrote:
> Can we just init "disconnect" right here?:
> 
> ```
> const bool disconnect = comm->GetCloseOnEOF();
> ```
> 
That won't work because we there are paths out of this function which should not disconnect (even in the close-on-eof case). For example, the read thread can be disabled by setting `comm->m_read_thread_enabled = false` in which case we should exit even though we have not reached EOF.


================
Comment at: lldb/source/Core/Communication.cpp:322-327
     if (bytes_read > 0)
       comm->AppendBytesToCache(buf, bytes_read, true, status);
     else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
-      if (comm->GetCloseOnEOF())
-        comm->Disconnect();
+      disconnect = comm->GetCloseOnEOF();
       comm->AppendBytesToCache(buf, bytes_read, true, status);
     }
----------------
clayborg wrote:
> Is there a reason we call AppendBytesToCache with zero bytes in the else? If we do need to call it (it might listen to the "status"?) then this:
> 
> ```
> if (bytes_read > 0)
>   comm->AppendBytesToCache(buf, bytes_read, true, status);
> else if ((bytes_read == 0) && status == eConnectionStatusEndOfFile) {
>   disconnect = comm->GetCloseOnEOF();
>   comm->AppendBytesToCache(buf, bytes_read, true, status);
> }
> ```
> can just be:
> 
> ```
> comm->AppendBytesToCache(buf, bytes_read, true, status);
> ```
> 
> bytes_read is a size_t so it can't be less than zero. Then we can move the "disconnect = comm->GetCloseOnEOF();" into the eConnectionStatusEndOfFile case in the switch below.
`AppendBytesToCache` calls a callback and I believe the idea is to call it with a 0-byte buffer on EOF. But the refactor looks good. I've made the `AppendBytesToCache` call conditional on `bytes_read > 0 || status == EOF` to preserve the existing behavior of not calling the callback in case of errors.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77295