Sarcasm / irony-mode

A C/C++ minor mode for Emacs powered by libclang
GNU General Public License v3.0
904 stars 99 forks source link

irony-server on Windows sometimes fails with "error: missing newline for unsaved file content" #251

Closed rochg closed 8 years ago

rochg commented 8 years ago

I have been setting up Irony on Mac and Windows. I am using Irony as a backend for Flycheck and Company. On Windows I found that while things did function correctly for a short while I would intermittently see the "missing newline for unsaved file content" error output from the Irony server and the server would exit.

I have tried to debug what was going on by adding diagnostics in both the server and the elisp. My conclusion was that the server was correct in that it really had not received the extra newline it expected. Instead of the newline character it was seeing the first character of the next request e.g. a 'p' to start a parse request.

Looking in the elisp, I noticed that when requests such as 'parse' are sent to the server the request is sent in three parts by irony--send-parse-request i.e. it calls process-send-string, process-send-region and then process-send-string again. Reading around how subprocess communication works in Emacs and the specific Windows issues already discussed on these pages (w32-pipe-read-delay etc) I started wondering if the problem was as a result of pipe buffers filling up. Specifically, I am thinking about the page http://www.gnu.org/software/emacs/manual/html_node/elisp/Input-to-Processes.html and where it says:

Sometimes the system is unable to accept input for that process, because the input buffer is full. When this happens, the send functions wait a short while, accepting output from subprocesses, and then try again. This gives the subprocess a chance to read more of its pending input and make space in the buffer. It also allows filters, sentinels and timers to run—so take account of that in writing your code.

I am wondering if Irony was being asked to perform a second request while part way through sending the first request. In support of this it often, although (I don't think) always, appeared to be a completion request from Company that was arriving before the end of parse request from FlyCheck. Maybe Company is being invoked by a timer/filter/sentinel when FlyCheck's request is only partially sent?

The above was just my theory of the problem - feel free to rule it out as I'm not expert in the code.

I did come up with a patch to send the request in a single call, which seems to have completely resolved the problem for me:

--- irony.el    2015-11-16 15:12:30.198143000 +0000
+++ /cygdrive/c/Users/rochg/.emacs.d/elpa/irony-20150831.144/irony.el   2015-11-16 15:15:11.050025500 +0000
@@ -685,19 +690,18 @@
       (irony--server-process-push-callback process callback)
       ;; skip narrowing to compute buffer size and content
       (irony--without-narrowing
+        ;; always make sure to finish with a newline (required by irony-server
+        ;; to play nice with line buffering even when the file doesn't end with
+        ;; a newline)
         (process-send-string process
-                             (format "%s\n%s\n%s\n%d\n"
+                             (format "%s\n%s\n%s\n%d\n%s\n"
                                      (combine-and-quote-strings argv)
                                      (combine-and-quote-strings compile-options)
                                      buffer-file-name
-                                     (irony--buffer-size-in-bytes)))
-        (process-send-region process (point-min) (point-max))
-        ;; always make sure to finish with a newline (required by irony-server
-        ;; to play nice with line buffering even when the file doesn't end with
-        ;; a newline)
-        (process-send-string process "\n")))))
+                                     (irony--buffer-size-in-bytes)
+                                     (buffer-substring (point-min) (point-max))))))))
+

-
 ;;
 ;; Buffer parsing
 ;;

Let me know if I can do anything further to help. Thanks so much for Irony and Flycheck - great stuff.

Sarcasm commented 8 years ago

Thank you for your enlightening issue. I remember seeing that process-send-string caused some "reentry" issue to some dude, now I understand better, I have to send the requests "atomically".

Out of curiosity, what is the output of M-x emacs-version RET? In Emacs 24.2 there was some fixes about process-send-{string,region} following this bug https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10815

Maybe Company is being invoked by a timer/filter/sentinel when FlyCheck's request is only partially sent?

I'm not sure but I think flycheck "floods" with request sometime, when you are still typing, it will sent 2 requests in a row because the delay is so short and parsing takes time. I think you said you had a "parse" request interrupting, the 'p'. So I don't think it's company, the request would be complete, 'c'.

Your fix makes sense. I'm wondering what is best, to everything as a string, or to create a temporary buffer for the request.

Sarcasm commented 8 years ago

Would you mind making a PR for this so the code can be attributed to you?

rochg commented 8 years ago

Sorry not to get back to you yesterday.

Out of curiosity, what is the output of M-x emacs-version RET? In Emacs 24.2 there was some fixes about process-send-{string,region} following this bug https://debbugs.gnu.org/cgi/bugreport.cgi?bug=10815

GNU Emacs 24.5.1 (i686-pc-mingw32) of 2015-04-11 on LEG570

Maybe Company is being invoked by a timer/filter/sentinel when FlyCheck's request is only partially sent?

I'm not sure but I think flycheck "floods" with request sometime, when you are still typing, it will sent 2 requests in a row because the delay is so short and parsing takes time. I think you said you had a "parse" request interrupting, the 'p'. So I don't think it's company, the request would be complete, 'c'.

I went back and looked at the logs I have for when the error happened, which was on 21 occaisons. The letter read instead of the newline was as follows:

So, it looks like there were cases when the request that got in before the newline came from Company and and other cases when it was FlyCheck.

Your fix makes sense. I'm wondering what is best, to everything as a string, or to create a temporary buffer for the request.

Yes, I wasn't sure here. I did share your concern that we are sometimes going to be creating a very big string and that there might be some performance cost. I don't know though if a temporary buffer would be any better - I haven't written enough lisp to be certain.

Would you mind making a PR for this so the code can be attributed to you?

OK, thanks. I will create the PR and you can either accept it or we can work an alternative if temporary buffers sound better. I will add a better comment than is in the patch above to explain about the atomic requirement we're seeing.

Sarcasm commented 8 years ago

Any chance you make the PR?

rochg commented 8 years ago

Sorry for the delay. The PR is there now - just let me know if I did anything stupid as it's the first contribution I've made through GitHub. #256

Sarcasm commented 8 years ago

Hi, thanks, I merged it, it looked good.

I just edited the commit message, in git usually the first line is short, then an empty line and a more elaborate description can follow.

Thanks you for finding and fixing this bug!

weirdNox commented 8 years ago

Hello there, this bug is really weird...

I am still having this error: missing newline for unsaved file content when trying to complete (it seems that it is random, however I believe it is more frequent when completing struct/class members). I'm using Windows 10 and have the 64-bit version of LLVM (compiled Irony using -G "Visual Studio 14 Win64" in order to find clang).

I was using emacs-w64 but just tried it in the official build (GNU Emacs 24.5.1 (i686-pc-mingw32) of 2015-04-11 on LEG570) and also get the error.

On the Messages buffer, I got this message repeated some times:

Company: An error occurred in auto-begin
Company: backend company-irony async timeout with args (candidates )

When I retry to auto-complete, it will eventually print this:

irony process stopped!
Company: An error occurred in auto-begin
Writing to process: bad file descriptor, Irony

I'm really new to emacs, just started using it, so I'm not fluent in elisp. I get the error using company-irony and irony-completion-at-point-async.

So, don't know if this problem is associated with this issue at all.. :/ If you need more information or want me to try anything, just ask! Thanks.