Page MenuHomePhabricator

Rewrite JSON dispatcher loop using C IO (FILE*) instead of std::istream.
ClosedPublic

Authored by sammccall on Jun 1 2018, 8:59 AM.

Details

Summary

The EINTR loop around getline was added to fix an issue with mac gdb, but seems
to loop infinitely in rare cases on linux where the parent editor exits (most
reports with VSCode).
I can't work out how to fix this in a portable way with std::istream, but the
C APIs have clearer contracts and LLVM has a RetryAfterSignal function for use
with them which seems battle-tested.

While here, clean up some log() usages to avoid double-newlines (except after large JSON payloads)

Diff Detail

Event Timeline

sammccall created this revision.Jun 1 2018, 8:59 AM

@malaperle: would you mind patching this in and checking whether attaching a debugger still works on Mac (this was the reason for the EINTR loop, right?)

I want to preserve this but now people other than me are complaining about old clangds hanging around and eating all their CPU :-)

@malaperle: would you mind patching this in and checking whether attaching a debugger still works on Mac (this was the reason for the EINTR loop, right?)

I want to preserve this but now people other than me are complaining about old clangds hanging around and eating all their CPU :-)

I tried the patch but as soon as I attach the debugger I get "IO error: Interrupted system call" from Clangd's stderr. I didn't get to look into this further.

sammccall updated this revision to Diff 149682.Jun 4 2018, 12:54 AM

clearerr() on (partial) successful read, and recover from EAGAIN during fread.

@malaperle: would you mind patching this in and checking whether attaching a debugger still works on Mac (this was the reason for the EINTR loop, right?)

I want to preserve this but now people other than me are complaining about old clangds hanging around and eating all their CPU :-)

I tried the patch but as soon as I attach the debugger I get "IO error: Interrupted system call" from Clangd's stderr. I didn't get to look into this further.

Thanks! I managed to kind-of reproduce this (kill -SIGSYS produces the same behavior... in some cases) and it turns out I forgot to clear the error bit on the stream.
If you get a chance to try again it'd be much appreciated (or @ilya-biryukov, you have a mac laptop right?)

sammccall updated this revision to Diff 149685.Jun 4 2018, 1:41 AM

Handle one more case where fgets is interrupted.

The change LG, but I'm not a big expert on C APIs, so I might've missed something. @ioeric, @hokein, maybe you have some experience with those and want to take a look?
PS I've checked it on my Mac and lldb seems to attach just fine.

clangd/JSONRPCDispatcher.cpp
70

Log adds a newline for us, right? Why do we want two newlines here?

sammccall edited the summary of this revision. (Show Details)Jun 4 2018, 7:15 AM
sammccall added inline comments.Jun 4 2018, 7:18 AM
clangd/JSONRPCDispatcher.cpp
70

Oops, forgot to mention in the description... Yes this is a deliberate double-newline.

While I was here, I removed the trailing \n in most log() calls, but decided to *keep* the one after <--and *add* one after -->, because they help visually separate the big JSON payloads from the rest of the log, which is useful while scanning.

Happy to remove these special cases, or back out all the unrelated newline changes if you prefer.

PS I've checked it on my Mac and lldb seems to attach just fine.

Working fine in my setup too!

ilya-biryukov accepted this revision.Jun 5 2018, 1:20 AM

LGTM

clangd/JSONRPCDispatcher.cpp
70

It's fine. Just wanted to make sure this wasn't accidental.

This revision is now accepted and ready to land.Jun 5 2018, 1:20 AM
This revision was automatically updated to reflect the committed changes.