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

Fix for 794 (chunking input on Windows) #850

Merged
merged 1 commit into from Jan 14, 2016

Conversation

@cpitclaudel
Copy link
Member

commented Jan 11, 2016

Here's the PR :)

Edit: Connects to #794

flycheck.el Outdated
(process-send-region process (point-min) (point-max))
(process-send-eof process)))
(flycheck--process-send-buffer process)
(process-send-eof process))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jan 11, 2016

Contributor

I think I'd prefer to make --process-send-buffer send the EOF as well. Let's have everything at just one place :)

flycheck.el Outdated
(save-restriction
(widen)
(if (eq system-type 'windows-nt)
(let ((from (point-min)))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jan 11, 2016

Contributor

I think we should move all of this into a separate function, e.g. flycheck--process-send-buffer-contents-chunked. The function name will be easier to understand than the code itself; it'll help us to understand what it's for in the future.

Furthermore, I think we should move the system-type test out of this function into a defvar, e.g. flycheck-chunked-process-input or so, that holds the value of the system-type test. This lets users easily switch between both, which will help us to test the code on systems other than Windows (e.g. to fix a bug therein), and users to switch if we ask them to, e.g. to check whether their version of Emacs or Node.js is still affected by the bug.

What do you think?

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jan 11, 2016

Contributor

PS: I don't think that we should have a proper defcustom for this, for most users should never ever need to change it, or would want to. A simple defvar should be sufficient, in my opinion.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Jan 11, 2016

Author Member

I think your code reviews are great, as usual. Will update the PR.

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2016

Done :)

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

@cpitclaudel Thanks. I pushed a commit that shortens the docstrings.

However, following the discussion in the corresponding Emacs bug, I'm not sure whether I still like to default to chunking generally. We probably have to since we can't reasonably choose more specifically, but I do not like to degrade performance for all Windows users that aren't affect. Python, for instance, seems to be fine.

Did you check whether there's a node.js issue? I didn't find any. Should we probably report it to node.js, too?

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2016

However, following the discussion in the corresponding Emacs bug, I'm not sure whether I still like to default to chunking generally. We probably have to since we can't reasonably choose more specifically, but I do not like to degrade performance for all Windows users that aren't affect. Python, for instance, seems to be fine.

Yeah, I'm not sure either. I don't really like this kind of bugs. I don't know if there's a way to detect this situation, either. I don't want this small windows tweak to start rippling through the codebase though, so I won't champion per-checker settings :)

I can, however, test out the performance of that thing. Maybe it's not that bad.

Did you check whether there's a node.js issue? I didn't find any. Should we probably report it to node.js, too?

I didn't find anything either :/ A report would probably be in order, but I'm not sure if now is the best time. We might want to wait until things are a bit clearer on the Emacs side; then (or if things are not making progress) we can probably ping the node.js people. What do you think? We could also ask them right now.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

But I'm afraid that this kind of a hack will stick forever in our code because none ever looks at it again. I don't want to end up there.

Agreed.

Hence, I think we should make it the default on Windows now, but add a TODO with a date to re-evaluate the situation in January 2017. By that time it should have been reported to node.js and fixed. If it's not, I'd just remove the fix again, and simply declare node.js on Windows broken. That'd at least put up pressure on anyone with Windows and Node.js experience to properly track down and fix the issue, and we can't live with workarounds forever.

I think it's a very reasonable timeline. Especially since the bug seems to be fixed on Emacs' side, and Emacs25 will be out soon, so people will have had time to update by then.

I can, however, test out the performance of that thing. Maybe it's not that bad.

That'd be great 👍

I'll try to do that soon. In the meantime I tried out the patch, and it seems to fix the issue!

A report would probably be in order, but I'm not sure if now is the best time. We might want to wait until things are a bit clearer on the Emacs side; then (or if things are not making progress) we can probably ping the node.js people. What do you think? We could also ask them right now.

I think it's better to report early. We'll probably get node.js people to add their opinion and research to the corresponding Emacs bug, or probably discover a bug in their own code.

Can you do that? I don't have a Windows system to test anything on; I'm afraid I'm of little help in this issue 😞

I will; I'm waiting for more information on Eli's fix to see if this is something worth mentioning to the node.js people, or whether it was all on Emacs' side.

@lunaryorn lunaryorn referenced this pull request Jan 13, 2016
@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@cpitclaudel Given that the issue appears to have been fixed in Emacs 25 now, we can safely merge this workaround as there's a clear path for obsoleting and removing it again, namely as part of our Emacs 25.1 cleanup milestone.

Based on Eli's commit I'd adapt the condition for enabling the workaround though, in order to skip it if the issue is fixed upstream.

flycheck.el Outdated
(defvar flycheck-chunked-process-input
;; See https://github.com/flycheck/flycheck/issues/794 and
;; https://debbugs.gnu.org/cgi/bugreport.cgi?bug=22344
(eq system-type 'windows-nt)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Jan 14, 2016

Contributor

@cpitclaudel Following Eli's commit that fixed the issue properly we should update the condition to not enable our work around on Emacs 25.1.

The commit appears to introduce a new w32-pipe-buffer-size option, which we could use as feature test for this fix, and thus change the condition to (and (eq system-type 'windows-nt) (not (boundp 'w32-pipe-buffer-size))).

What do you think?

Besides, what about cygwin? Did any of the reporters of #794 observe the issue on cygwin?

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Jan 14, 2016

Author Member

This sounds great; feel free to go ahead with this if you have time before I do. I just tried on Cygwin quickly, but node.js was getting confused by cygwin's path format (/cygdrive/c/...).

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2016

I opened an issue with Node.js at nodejs/node#4697

@lunaryorn lunaryorn force-pushed the 794-chunked-stdin-on-windows branch from a5876ef to 7265f2f Jan 14, 2016

lunaryorn added a commit that referenced this pull request Jan 14, 2016

Chunk process input on Windows
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.
Chunk process input on Windows
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.

@lunaryorn lunaryorn force-pushed the 794-chunked-stdin-on-windows branch from 606e54e to cfee856 Jan 14, 2016

@lunaryorn lunaryorn merged commit cfee856 into master Jan 14, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@lunaryorn lunaryorn deleted the 794-chunked-stdin-on-windows branch Jan 14, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2016

@cpitclaudel I merged the fix into master, with modifications as discussed.

May I ask you to test the next MELPA build on Windows in a broken and a fixed Emacs version, and check whether the issue is gone, and whether flycheck-chunked-process-input is really nil on a fixed build of Emacs? That'd be really awesome 😊

I opened #856 for us to track the removal of the workaround once we drop support for Emacs 24, in our Emacs 25 cleanup milestone.

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2016

Will do :)

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2016

All checks passed :)

  • ✓ Reproduced the bug in Emacs 24.5 before updating Flycheck
  • ✓ Updated Flycheck and confirmed that the bug was gone and that flycheck-chunked-process-input was t there
  • ✓ Made sure that the bug was gone in Emacs 25 and that flycheck-chunked-process-input was nil there

My, that was a tricky bug.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2016

@cpitclaudel Thank you! That's great news! You did awesome work on tracking down and fixing this bug! Thank you very much; Flycheck and I owe you a lot!

@cpitclaudel

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2016

:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.