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

stuck when tramp connection gets lost #883

Closed
abred opened this Issue Feb 24, 2016 · 6 comments

Comments

Projects
None yet
2 participants
@abred
Copy link

abred commented Feb 24, 2016

Hello,

situation:
global-flycheck-mode is on. helm is on.
I'm editing files via ssh/tramp. The connection gets lost (for some reason). Sometimes tramp recovers, sometimes not. If not, I try to execute tramp-cleanup-all-connections to reconnect.

what is supposed to happen:
I press M-x (aka helm-M-x), type tramp-cleanup-all-connections and execute it.

what does happen:
I press M-x (aka helm-M-x). emacs freezes. This happens because a "file-exists-p" request is sent do the remote server (which is currently unavailable).
output of debug/tramp buffer (tramp-verbose 8):
19:20:18.872941 tramp-get-file-property (8) # /home/user/folder/ file-exists-p t
(Also, when the connection is alive and I do M-x, I get one of these lines for every keystroke, if the current buffer is a remote one)

why does this happen:
On pressing M-x a buffer called "helm-M-x" is created. Because global-flycheck-mode is activated, there is a check if flycheck should be activated for this new buffer.
The check succeeds (<- Problem 1)
After flycheck is activated, it checks if the file exists, however
this check is (apparently) sent to the remote server (<- Problem 2) (if the buffer, which is active when I press M-x, is a remote buffer)

suggestions:
Problem 1:

  • treat buffers starting with a * as ephemeral buffers
  • add a variable with a list of regex, if a buffer name matches the regex, don't activate flycheck
  • only activate flycheck if buffer-file-name is non-nil

Problem 2:

  • don't send the file-exists-p check to the remote server
    This seems to be a separate issue as the helm-M-x buffer is a local one and not a remote one.

GNU Emacs 24.5.1 (x86_64-redhat-linux-gnu, GTK+ Version 3.16.6) of 2015-09-14 on buildvm-10.phx2.fedoraproject.org
Flycheck version: 0.26snapshot (package: 20160222.551)

(ref: emacs-helm/helm#1398)

lunaryorn added a commit that referenced this issue Feb 24, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Feb 24, 2016

@abred Thank you for your report, and also for your suggestions. But please understand that the problem is more intricate and that your suggestions are not applicable at all:

  • A buffer starting with * is not simply not ephemeral—only buffers starting with a whitespace are. There are many legitimate cases in which Flycheck should absolutely check buffers starting with *—Org's source edit buffers for instance. Besides, Helm's buffers aren't ephemeral either because the user is actually expected to interact with helm buffers—that's the entire point and purpose of helm.
  • I am not at all in favour of adding special cases to work around issues in other packages.
  • Flycheck explicitly supports checking buffers without a backing file; that's a feature.

As for the check, we must check whether default-directory exists to be able to run subprocesses. I've added a check that refuses to check if default-directory is remote—which effectively disables Flycheck for all Tramp buffers. This should hopefully fix the immediate issue.

The root cause however is Helm, I'd argue. It should mark its major modes and hence its buffers as special, see Major Mode conventions:

If this mode is appropriate only for specially-prepared text produced by the mode itself (rather than by the user typing at the keyboard or by an external file), then the major mode command symbol should have a property named mode-class with value special,

@abred

This comment has been minimized.

Copy link

abred commented Feb 24, 2016

Thanks for your explanations.

But I am not sure your commit is the right way to go. In general flycheck works just fine for remote buffers.
Your remark about major modes sounds more promising.

lunaryorn added a commit that referenced this issue Feb 24, 2016

Revert "Don’t check when default-directory is remote"
Don’t actively break Tramp support.  We need to find a different
solution for GH-883.

This reverts commit aad1de2.

lunaryorn added a commit that referenced this issue Feb 24, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Feb 24, 2016

@abred Does it indeed? I'm surprised to hear that; note that we don't support Tramp officially. If it works, it does so by accident.

Still, I've reverted the corresponding change, and instead—following the discussion in emacs-helm/helm#1398—disabled Flycheck in fundamental-mode buffers. Therry is right, it doesn't make sense in these buffers anyway, and presumably Helm uses fundamental-mode for its buffers so this change should fix this issue.

Nonetheless I think that tramp should definitely define its own major mode for its buffers. That's simply good style and courtesy towards other Emacs extensions.

@lunaryorn lunaryorn added kind: bug and removed status: blocked labels Feb 24, 2016

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Feb 24, 2016

@abred Would you mind to check whether the issue is gone after the next MELPA rebuild? I don't use Tramp at all, so I'm not able to actually test this change.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

lunaryorn commented Feb 25, 2016

Helm now defines its own major mode for its buffers as well. Flycheck should not become active in Helm buffers anymore in any version of Helm. I'm closing this issue; please leave a comment if the issue still persists so that we can investigate further.

Thank you for reporting this issue and for your help and feedback in getting it fixed 👍

@abred

This comment has been minimized.

Copy link

abred commented Feb 25, 2016

is fixed, thanks! :)

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