Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

flycheck hangs with eslint #794

Closed
bbigras opened this issue Nov 16, 2015 · 57 comments
Closed

flycheck hangs with eslint #794

bbigras opened this issue Nov 16, 2015 · 57 comments

Comments

@bbigras
Copy link
Contributor

bbigras commented Nov 16, 2015

I have 80 errors in my file if I run eslint from the command line. I tested with a smaller file (with less errors) and it was fine. Emacs has 0% cpu usage and I don't see eslint in the process manager. I think it's related to eslint since the file opens if I remove eslint from my PATH.

There's nothing in the message buffer.

GNU Emacs 24.5.1 (x86_64-w64-mingw32) of 2015-05-16 on KAEL

@swsnr
Copy link
Contributor

swsnr commented Nov 16, 2015

Please tell us your Flycheck version (M-x flycheck-version) and your eslint version, and please check whether you are able to reproduce the issue in emacs -Q. It'd be great if you could produce a step-by-step recipe to reproduce the issue in emacs -Q.

Please note that we do not support Windows. I'm closing this issue as invalid until we're able to reproduce it on a Unix system.

@bbigras
Copy link
Contributor Author

bbigras commented Nov 16, 2015

Flycheck version: 0.25snapshot (package: 20151113.858)
eslint v1.9.0
node v5.0.0

I can't reproduce with emacs -Q.

@swsnr
Copy link
Contributor

swsnr commented Nov 16, 2015

In other words, if you start emacs -Q, enable global-flycheck-mode and visit the affected file Flycheck works fine and highlights all issues in the file?

@bbigras
Copy link
Contributor Author

bbigras commented Nov 16, 2015

Sorry I didn't realise you wanted me to load flycheck manually.

emacs hangs when I visit the affected file after loading flycheck manually like this :

(add-to-list 'load-path "~/.emacs.d/elpa/dash-20151021.113")
(add-to-list 'load-path "~/.emacs.d/elpa/flycheck-20151113.858")
(require 'flycheck)
(global-flycheck-mode)

@swsnr
Copy link
Contributor

swsnr commented Nov 16, 2015

@BrunoQC Uhm, yes, I wanted you to actually try and see whether Flycheck works in emacs -Q. Sorry for the confusion.

Can you given me the file which shows this issue? I'd like to try myself. Alternatively, can you try to reproduce the problem on a Unix system? That's somewhat crucial, for otherwise I'm unable to debug and fix the issue. If it's a Windows-only problem, you'll have to debug that yourself.

Besides, you're not using the latest Flycheck build from MELPA, please consider updating.

@bbigras
Copy link
Contributor Author

bbigras commented Nov 16, 2015

I have the problem with https://ajax.googleapis.com/ajax/libs/angularjs/1.4.7/angular.js . I also tested with an up-to-date flycheck.

I wasn't able to reproduce on Debian.

Debian GNU/Linux 8.2 (jessie)
GNU Emacs 24.5.1 (x86_64-pc-linux-gnu, GTK+ Version 3.16.4) of 2015-06-28 on trouble, modified by Debian
Flycheck version: 0.26snapshot (package: 20151116.124)
eslint v1.9.0
node v5.0.0

I'll test at home later today on a faster and better Windows machine.

@bbigras
Copy link
Contributor Author

bbigras commented Nov 17, 2015

I also have the problem at home.

@swsnr
Copy link
Contributor

swsnr commented Nov 17, 2015

Also on a Windows machine? I'm inclined to believe that it's s Windows specific issue. Then you're on your own.

I'm sorry but I can't and don't want to fix Windows issues. I don't have access to a Windows development machine and it's too alien to me to spend a lot of time with.

Sorry.

@swsnr
Copy link
Contributor

swsnr commented Nov 17, 2015

@BrunoQC I'm also unable to reproduce this issue on a Linux system. I'm sorry.

@bbigras
Copy link
Contributor Author

bbigras commented Nov 17, 2015

Also on a Windows machine?

Yes.

I'm sorry but I can't and don't want to fix Windows issues.

No problem.

I checked around a bit even if I don't know lisp (or is it elisp?) and I have a workaround that work for me in case anyone else has the same problem.

It works if flycheck call this Go program which only call eslint. It's weird that it doesn't work with cmd.Stdin = os.Stdin.

I'll check again another day.

:command ("my.exe" "eslint" "--format=checkstyle"
package main

import (
    "io"
    "os"
    "os/exec"
)

func runCMD() error {
    cmd := exec.Command(os.Args[1], os.Args[2:]...)
    // cmd.Stdin = os.Stdin // doesn't work
    cmd.Stdout = os.Stdout

    pipe, errPipe := cmd.StdinPipe()
    if errPipe != nil {
        return errPipe
    }

    if err := cmd.Start(); err != nil {
        return err
    }

    if _, err := io.Copy(pipe, os.Stdin); err != nil {
        return err
    }

    if err := pipe.Close(); err != nil {
        return err
    }

    cmd.Wait() // ignore the return code

    return nil
}

func main() {
    err := runCMD()
    if err != nil {
        panic(err)
    }
}

@swsnr
Copy link
Contributor

swsnr commented Nov 17, 2015

@BrunoQC You're not using cygwin, are you?

@bbigras
Copy link
Contributor Author

bbigras commented Nov 17, 2015

I have it installed but I don't use it. It's not in my path. I may have some gnu .exe like sh.exe in my path.

@swsnr
Copy link
Contributor

swsnr commented Nov 17, 2015

I'm at a loss then. I've no clue what could possibly cause this issue. My Windows foo is pretty bad, though.

Out of curiosity: Could you try another Javascript checker, e.g. jshint or standard? And if those hang too, could you try some other checker from Flycheck? Doesn't matter what language or what tool, but it should have :standard-input t, because that is what changed of late.

@bbigras
Copy link
Contributor Author

bbigras commented Nov 17, 2015

jshint did hang but not gofmt and flake8.

@swsnr
Copy link
Contributor

swsnr commented Nov 17, 2015

Well, that's smells like a Node on Windows issue then. Is there a known issue about standard input on Windows or something like that?

@bbigras
Copy link
Contributor Author

bbigras commented Nov 17, 2015

Is there a known issue about standard input on Windows or something like that?

I have no idea.

@Dogfalo
Copy link

Dogfalo commented Nov 30, 2015

@BrunoQC, I would try using an older version of flycheck, I also encountered the same problem but my old version flycheck-20150921.947 worked fine.

@cpitclaudel
Copy link
Member

I might be able to look at this quickly on a Windows machine. @Dogfalo @BrunoQC Which version of the OS were you using?

@Dogfalo
Copy link

Dogfalo commented Nov 30, 2015

I am on windows 10 and emacs 24.4

@bbigras
Copy link
Contributor Author

bbigras commented Nov 30, 2015

Windows 10
Emacs 24.5.1 (x86_64-w64-mingw32) of 2015-05-16 on KAEL

@Xeveo
Copy link

Xeveo commented Dec 17, 2015

I'm having the same issue.

  • Windows 7
  • emacs 24.5.1
  • eslint v1.10.3
  • jshint v2.8.0

I used git bisect to find the commit where the issue was introduced:

Oct 25 - 6088731 - Use standard input for javascript checkers.

I'm a newbie when it comes to elisp, so I'm afraid I won't be of much use to actually suggest a fix... but at the very least, we've narrowed it down to a few small changes made in a single commit.

Hopefully there haven't been too many changes since then that rely on this commit, and fixing it will be easy.

In the mean time, anyone else having this problem can checkout c2e413a. That's the latest commit that should be working correctly on Windows.

@cpitclaudel
Copy link
Member

Hi Xeveo,

Thanks for bisecting; I haven't gotten around testing this yet. I think @lunaryorn identified this problem in a previous message, mentioning stdin checkers. A fix will certainly not be very easy, depending on the extent of the problem.

As a workaround, you could just copy the checker definition from that commit, instead of reverting to a previous commit. The issue seems to be with the way the checkers in question deal with being fed input on standard input instead of as a separate file.

@Xeveo
Copy link

Xeveo commented Dec 17, 2015

@cpitclaudel

Thank you for the quick response. As you and @lunaryorn suggested, the issue seems to be a problem with standard-input.

Reverting the relevant changes from the commit I mentioned fixes the issue.
I've forked, branched from master, and committed the change, and all seems to be well.
8b472e5 - Standard Input Revert

I don't want to create a pull request, because I'm really not familiar with the code, and as I said in my previous comment, I'm very new to the language. Besides, the changes in 6088731 were made for a reason. If it works on Unix/Linux systems, then there's probably an alternate solution that would be more appropriate than reverting (probably not worth your time to investigate, as it seems to be an emacs bug).

@bschwehn
Copy link

Just wanted to comment that I had the same issue on my windows machine (working fine on my linux systems) and the patch by Xeveo seems to have solved my issue.

@wsw0108
Copy link

wsw0108 commented Dec 28, 2015

@BrunoQC I had the same problem, but with jshint.cmd.

And I found that if the length of buffer/file less than or equals to 4096 bytes, jshint works fine, otherwise jshint hangs emacs...

I guess this may caused by write() in mingw-msys that build emacs or the stream impl(using windows pipe) of nodejs.

@baerrach
Copy link

baerrach commented Jan 7, 2016

I started investigating this because I didn't know the cause, and when I found the issue occurred at 4096, I was able to find this issue.

The link provided by @Xeveo has resolved the issue.

Having found what will cause the problem, but not what actually causes the problem, and a valid workaround, I will live with the workaround.

If its possible to include this problem in the documentation that would be helpful.

baerrach added a commit to baerrach/.emacs.d that referenced this issue Jan 7, 2016
Add flycheck-pos-top, use elisp xml-parse.
Check on save only (hack until fix for 4096 eshint hanging issue
resolved flycheck/flycheck#794)
@swsnr
Copy link
Contributor

swsnr commented Jan 13, 2016

Thank you all very much for your help with tracking down the cause, and for your thorough testing of the fix. We appreciate your help, and a very happy to work with such great people! Thank you very much.

I'd like to inform you that following @cpitclaudel's bug report to Emacs upstream a potential fix for this issue was committed to the emacs-25 branch (thank you very much, Eli!).

If you have some time to spare, and would like to help Emacs and Flycheck, it would be awesome if you could compile the latest emacs-25 including commit 58a622d on Windows and test whether the issue still persists with the current Flycheck version. It'd help us very much if we could confirm that the fix does indeed fix the issue on Emacs' side.

Thank you very much.

@bschwehn
Copy link

I can no longer reproduce the hang using a build that includes 58a622d, so it looks to me that the issue is fixed.

Thanks
Ben

@bbigras
Copy link
Contributor Author

bbigras commented Jan 13, 2016

@bschwehn Do you have instructions how to build emacs on Windows or did you use a snapshot from somewhere?

@bschwehn
Copy link

@BrunoQC I used the instructions from here:

http://sourceforge.net/p/emacsbinw64/wiki/Build%20guideline%20for%20MSYS2-MinGW-w64%20system/

Also in the source tree, the nt folder has some instructions. I had no issues (but make sure to have autocrlf turned off when checking out emacs, not working otherwise).

Just to make clear: I have so far not actually tried flycheck, just the hang repro steps in the bug report.

I was able to repro the hang using the steps that Clément described in the emacs bug report and can no longer repro using my latest build.

I will try with flycheck tomorrow.

@bschwehn
Copy link

Also, if it helps anyone, I can put the build up somewhere. Let me know.

@Xeveo
Copy link

Xeveo commented Jan 13, 2016

I can also confirm that the issue seems to be fixed by the 58a622d commit.

@bschwehn
Copy link

@Xeveo: did you try with actually using flycheck in a scenario where it hanged before (undoing any workarounds you have applied)? I didn't have time to look at it further yet, but noticed that with the workarounds still in place (i.e. jshint is passed a file, not via stdin), flycheck still works, but I get a new warning message like:

Suspicious state from syntax checker javascript-jshint: Checker javascript-jshint returned non-zero exit code 2, but no errors from output: <?xml version="1.0" encoding="utf-8"?>
<checkstyle version="4.3">
    <file name="c:/Users/BENJAM~1.SCH/AppData/Local/Temp/flycheck31856JJr/app.routes.js">
        <error line="316" column="13" severity="warning" message="&apos;logger&apos; is not defined." source="jshint.W117" />
        <error line="18" column="17" severity="warning" message="&apos;log&apos; is defined but never used." source="jshint.W098" />
    </file>
</checkstyle>
Checker definition probably flawed.

Which I did not get before. Did you notice anything like this also? I don't think it is related to the 58a622d commit.

@Xeveo
Copy link

Xeveo commented Jan 13, 2016

@bschwehn: I checked out the lastest master content for flycheck, and opened a file that had previously caused emacs to hang (20,480 bytes in size). The file was linted correctly, and I did not get any warnings or errors from emacs.

I've tested with both javascript-jshint, and javascript-eslint.

@bbigras
Copy link
Contributor Author

bbigras commented Jan 13, 2016

@bschwehn
Thanks for the instructions.

It seems I still have the problem even with the 58a622d473112f8ff5b4bdb3e49bc6573dfd3404 commit.

I'm leaving work for today. I'll test again tomorrow.

@cpitclaudel
Copy link
Member

I've had a bit of time to recompile Emacs on Wdinows. The results of my tests were that a plain build (according to the instructions in nt/INSTALL) of the master branch does fail (as expected), but a build of the emacs-25 branch seems to work as expected.

@swsnr
Copy link
Contributor

swsnr commented Jan 14, 2016

Thank you all for your time spent on testing the fix. You're awesome people, the lot of you 😍 👏

I'll commit the workaround for Emacs 24 as soon as possible.

In order to correctly enable the workaround for all affected versions of Emacs and operating systems, I'd like to ask one final question, though:

Did any of you observe the issue on cygwin? Or could anyone test whether cygwin is also affected?

@bbigras
Copy link
Contributor Author

bbigras commented Jan 14, 2016

Disregard my last comment. I was on the wrong branch. Sorry about that.

The emacs-25 branch does fix the problem I had with eslint.

@bbigras
Copy link
Contributor Author

bbigras commented Jan 14, 2016

Did any of you observe the issue on cygwin? Or could anyone test whether cygwin is also affected?

I'm trying to test with cygwin but flycheck doesn't show the eslint errors.

  javascript-eslint
    - predicate:  t
    - executable: Found at /cygdrive/c/Users/bbigras/AppData/Roaming/npm/eslint

I wonder if it's because I'm using a non-cygwin node.js. eslint seems to work in the Cygwin64 Terminal.

node.js doesn't seem to be available as a cygwin package.

@cpitclaudel
Copy link
Member

Indeed, I had trouble trying this with Cygwin as well; node.js was confused by the path format for the config file (/cygdrive/...). Since the Emacs patch that fixed this issue will apply to Cygwin as well I'm not too too worried.

I opened an issue with the Node.js people, to get their input. We'll push the temporary fix to Flycheck soon, and the problem will be definitively fixed with the next release of Emacs. Phew.

swsnr pushed a commit that referenced this issue Jan 14, 2016
This is an Emacs bug: process-send-region just hangs when given more
than 4096 characters to process.  See the Emacs bug report at
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=22344 for a more complete
description.

The issue is fixed in Emacs 25.1, but we’ll add and keep this workaround
while we support Emacs 24.

Fixes GH-794 and closes GH-850.
@swsnr swsnr closed this as completed in cfee856 Jan 14, 2016
@swsnr
Copy link
Contributor

swsnr commented Jan 14, 2016

I merged @cpitclaudel's fix into master; it should be in the next MELPA build. Thank you very very much for your help and your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants