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.

Sign up for GitHub

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

Jump to bottom

Improve fish 4.0 shell support #4597

Closed
mc-butler opened this issue Oct 9, 2024 · 39 comments · Fixed by #4663
Closed

Improve fish 4.0 shell support #4597

mc-butler opened this issue Oct 9, 2024 · 39 comments · Fixed by #4663
Assignees
@zyv
Labels
area: core Issues not related to a specific subsystem prio: low Minor problem or easily worked around
Milestone
4.8.33

Comments

@mc-butler
Copy link

Important

This issue was migrated from Trac:

Origin https://midnight-commander.org/ticket/4597
Reporter zaytsev (@zyv)
Mentions aclopte@….com (@krobelus)

Patches from the mailing list:

v1:

v2:

Note

Original attachments:
@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 9, 2024 at 8:33 UTC (comment 1)

  • Description edited

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 9, 2024 at 8:45 UTC (comment 3)

Yes, I don't like the environment variable thing all... Is it possible for fish to parse our version from mc --version? If this is not practical, maybe we could consider exporting a generic MC_VERSION variable?

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 11, 2024 at 6:59 UTC (comment 4)

I just sent an updated patch series to the mailing list,
to use MC_VERSION instead.
That third patch is not absolutely necessary but it would be nice, to allow users to converge quicklier on the new keyboard input style.

Let me know if there is anything else for me to do; I realize I didn't add a changelog entry; not sure if that's necessary.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 11, 2024 at 8:03 UTC (comment 5)

Could you please just attach them here (git format-patch HEAD~~)? I'm not sure if there are any differences otherwise and it's really painful to save messages and upload. I will remove my first comment.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 11, 2024 at 8:04 UTC (comment 6)

  • Description edited

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 12, 2024 at 10:07 UTC

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 12, 2024 at 10:07 UTC

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 12, 2024 at 10:07 UTC

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 12, 2024 at 10:16 UTC (comment 7)

Ok done.
Indeed git format-patch is exactly what git send-email sends, so running git am will give the same result.
I wasn't familiar with Trac, maybe the "Reporting problems" section in the README should say that it's preferred to submit patches on Trac.
(I also somehow didn't receive an email notification for comments)

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 12, 2024 at 14:53 UTC (comment 8)

Thank you! I hope we solve the familiarity problem by dropping Trac one day... instead of adding more info to the README. But good point, need to check what it says now. Not very smart if it currently says to send patches to the mailing list, then ask people to post to Trac on the mailing list.

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 12, 2024 at 18:37 UTC (comment 9)

I personally enjoy using mailing lists but I can see how they are not for everyone. There's an initial barrier to entry that most potential contributors won't cross. Some projects offer multiple avenues for sending patches (for example both Github and a mailing list), I think that's often a good choice

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 22, 2024 at 6:29 UTC (comment 10)

Do you feel like there should be more thought spent on "MC_VERSION"?
If preferred, I can try parsing "mc --version" instead.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 22, 2024 at 6:57 UTC (comment 11)

Do you feel like there should be more thought spent on "MC_VERSION"?

I really don't like exporting it. It would be good to know what Andrew thinks about it...

Regarding lists, it's just an impractical way for us to keep track of patches right now. We need a central place, or things are bound to get lost or forgotten, as is currently the case with mailing lists and GitHub, and currently that central place is Trac. I hope that one day we'll get to GitHub and decommission everything else (Trac), but there's still a lot of work to be done to make that happen.

@mc-butler
Copy link
Author

Changed by ossi (@ossilator) on Oct 22, 2024 at 7:22 UTC (comment 12)

i suggest emulating what shells do: set the version variable without exporting it. that would imply injecting an additional command early during the subshell setup.

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 23, 2024 at 18:23 UTC

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 24, 2024 at 4:53 UTC (comment 13)

  • Cc set to aclopte@….com

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 26, 2024 at 16:39 UTC (comment 14)

  • Branch state changed from no branch to on review
  • Owner set to zaytsev
  • Status changed from new to accepted

Branch: 4597_fish_csi
Initial [0ea77d2]

I like the second option, agree with Andrew's fixups.

@mc-butler
Copy link
Author

Changed by andrew_b (@aborodin) on Oct 26, 2024 at 18:08 UTC (comment 15)

  • Branch state changed from on review to approved
  • Votes set to andrew_b

There is no need to write "Ticket #XXXX:" in the 2nd, 3rd, etc. commits in the branch.

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Oct 26, 2024 at 18:13 UTC (comment 16)

I see, I didn't realize there will already be a merge commit to group commits

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Oct 26, 2024 at 18:41 UTC (comment 17)

I will clean up, rebase and merge as soon as CI is in.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Nov 1, 2024 at 17:12 UTC (comment 18)

Rebased on current master so that this branch gets CI and cleaned up as promised.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Nov 1, 2024 at 17:14 UTC (comment 19)

Hmmm, build on macOS breaks:

common.c:185:53: error: initializer element is not a compile-time constant
static const size_t subshell_switch_key_csi_u_len = strlen (subshell_switch_key_csi_u);
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Nov 1, 2024 at 17:28 UTC (comment 20)

Could probably use sizeof(subshell_switch_key_csi_u) - 1 as a workaround.
Actually it seems better to get rid of this constant; it's used twice but one of those uses can be elided by switching from memcmp to strcmp (no other change should be necessary)

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Nov 1, 2024 at 17:34 UTC (comment 21)

I've pushed a fixup and it indeed fixed the CI! Actually, this sounds like a compiler bug to me. I can't reproduce it locally with clang 16, but it does fail on macOS inside GitHub. No other platform with clang is failing though (FreeBSD is fine)... oh well, if it works - so be it.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Nov 1, 2024 at 17:42 UTC (comment 22)

  • Status changed from accepted to testing
  • Resolution set to fixed
  • Votes changed from andrew_b to committed-master
  • Branch state changed from approved to merged

Merged to master as [fc3ad53] .

Thank you everybody!

@mc-butler
Copy link
Author

Changed by johannes (@krobelus) on Nov 1, 2024 at 17:45 UTC (comment 23)

Thanks! The corresponding change has been merged to fish as well.
LMK if there are more fish-related issues; I don't really use mc myself yet but I'm happy to help.

@mc-butler
Copy link
Author

Changed by zaytsev (@zyv) on Nov 1, 2024 at 18:35 UTC (comment 24)

  • Status changed from testing to closed

Fantastic, whenever I stumble upon a FISH ticket while triaging, I'll put you on CC. It's always of great help to have someone in the know... Schönes Wochenende!

@asl
Copy link

asl commented Mar 3, 2025

Unfortunately, situation became worse after upgrading from fish 3.7.2 to fish 4.0.

Typical steps to reproduce:

  • Run mc
  • Press Ctrl+O, bringing console
  • Press something, sometimes even "Enter" is enough
  • Press Ctrl+O again
  • Then all navigation become broken, no response from arrow presses, no reaction on key presses
  • Mouse clicks work though, and on clicks sometimes escaped strokes appear on terminal line like ;146;12m
  • Sometimes, what is on commandline become enough to be sent to terminal via "Enter", then everything is unfrozen

So, it looks like there might be some race somewhere? And some characters are not seen by mc and / or terminal breaking navigation? Any ideas how this could be debugged?

I'm running fish inside iTerm2.

@zyv
Copy link
Member

zyv commented Mar 3, 2025

@krobelus, could you please be so kind to take a look? We don't use Fish ourselves.

I'm not sure if this is related to your updates to the CSI parser in preparation for Fish 4.0, or if it's a completely new, unrelated issue, so I'm reluctant to reopen this ticket.

@asl
Copy link

asl commented Mar 3, 2025

I am fine to try to debug stuff by myself, but some guidelines would be welcome, as this is not something I am very familiar with.

@krobelus
Copy link
Contributor

krobelus commented Mar 3, 2025

I'll certainly try. Unfortunately I haven't been able to reproduce this.
I'll try with iTerm2 tomorrow.
Same for another apparent fish 4 regression where the second ctrl-o doesn't work.

I think it's possible that it's race conditions with SIGSTOP. We should try to insert sleeps in the right places to confirm.
I like to think that it's possible to get rid of SIGSTOP, since fish can run arbitrary commands from key bindings. So if that's really the issue, it shouldn't be a problem to replace it.. but I'm not super familiar with why it's being used.

As for debugging, I'll try to come up with some advice if I don't manage to reproduce it.
First try if it happens with fish_features=no-keyboard-protocols mc

@zyv
Copy link
Member

zyv commented Mar 3, 2025

Hey, thanks for the feedback! Please let me know if I should reopen if you think it's really the same issue, or feel free to open a new one and mention this one if it's similar, but something different.

@asl
Copy link

asl commented Mar 3, 2025 •
edited
Loading

As for debugging, I'll try to come up with some advice if I don't manage to reproduce it. First try if it happens with fish_features=no-keyboard-protocols mc

So, I manage to halt it again here, but the symptoms are different (so might be different issue here):

  • It took me a bit longer (pressing Ctrl+O, entering commands, etc.)
  • It seems to freeze indefinitely – mouse clicks do not go through, etc.

I believe I saw this case multiple times before (with fish 3.7.2), so might be different (older) issue

UPS: Yes, running fish_features=no-keyboard-protocols mc seems to workaround the first issue from #4597 (comment)

Trying to see what's going on with the second one...

@krobelus krobelus mentioned this issue Mar 3, 2025
Closed
@asl
Copy link

asl commented Mar 3, 2025

The second case is waiting in SIGSTOP:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGSTOP
  * frame #0: 0x000000018e79ccc0 libsystem_kernel.dylib`__sigsuspend + 8
    frame #1: 0x0000000100d32a08 mc`synchronize + 84
    frame #2: 0x0000000100d310e0 mc`feed_subshell + 564
    frame #3: 0x0000000100d320b4 mc`do_subshell_chdir + 1068
    frame #4: 0x0000000100cf472c mc`panel_do_cd_int + 172
    frame #5: 0x0000000100cf4644 mc`panel_do_cd + 20
    frame #6: 0x0000000100cf5dcc mc`panel_cd + 92
    frame #7: 0x0000000100cf7f64 mc`do_enter + 156
    frame #8: 0x0000000100cf6430 mc`panel_execute_cmd + 516
    frame #9: 0x0000000100d56380 mc`group_default_callback + 208
    frame #10: 0x0000000100d558b0 mc`dlg_process_event + 240
    frame #11: 0x0000000100d55c34 mc`dlg_run + 256
    frame #12: 0x0000000100ce83f8 mc`do_nc + 3540
    frame #13: 0x0000000100cca5fc mc`main + 1796
    frame #14: 0x000000018e450274 dyld`start + 2840

If I allow it to continue, then it resumes again. Does this ring any bell?

@krobelus
Copy link
Contributor

krobelus commented Mar 3, 2025

mc itself got SIGSTOP? That's surprising, I would have expected it to only be sent to the shell.

@asl
Copy link

asl commented Mar 3, 2025 •
edited
Loading

mc itself got SIGSTOP? That's surprising, I would have expected it to only be sent to the shell.

Well, it seems it is spinning here: https://github.com/MidnightCommander/mc/blob/master/src/subshell/common.c#L544

But it looks like the signal is sent to the parent process somehow, but not to the child?

But so far, I think all the cases of this freeze I seen is when I changed the directories.

@zyv zyv linked a pull request Mar 5, 2025 that will close this issue
Fix kitty keyboard protocol parsing of ctrl-o #4663
Merged
@asl
Copy link

asl commented Mar 6, 2025 •
edited
Loading

I have not followed the subshell logic fully as it is quite convoluted, but it seems there might be race in SIGCHLD hanlding. In particular, synchronize blocks SIGCHLD and then waits for a signal to arrive. However, if it happened to arrive before synchronize is called in feed_subshell, then it might stuck in waiting forever in sigsuspend loop.

For me it usually it happens during navigation (so do_subshell_chdir calls feed_subshell). It might require some directory changes, but I'm usually can reproduce trying to walk randomly over directories during minute or so.

I just verified, if I'd do kill -s CHLD <mc pid> when it hangs, then it unfreezes.

@zyv Does this ring any bell? What if subshell is fast enough to signal before synchronize?

@zyv
Copy link
Member

zyv commented Mar 7, 2025

@asl Thanks for the details! Yes, it does ring a bell, it seems to me that you're running into #4071, which is still open - and that's a completely different issue from what's being discussed here.

It could (unfortunately) very well be that there is a race, and it's not usually triggered by "fast" subshells like bash, but more often occurs with "slow" subshells like zsh or fish.

Sadly, it's not an easy bug to fix (at least not for me). The whole mc architecture has evolved over 30 years in an incredibly racy and brittle way, which mainly manifests itself in two directions. One is the interaction with the terminal, and the other is the interaction with the subshell.

We have plugged holes in the terminal interaction, and a little bit on the subshell side, but apparently the "right" solution would be a major refactoring introducing an event loop.

However, we don't have the resources for such an undertaking at the moment. If someone comes along and offers to plug a particular hole, we will try to get it in... Who knows, maybe you don't have enough challenges in your life and this might pique your interest :)

@asl
Copy link

asl commented Mar 7, 2025

@asl Thanks for the details! Yes, it does ring a bell, it seems to me that you're running into #4071, which is still open - and that's a completely different issue from what's being discussed here.

Yes, looks like it is another instance of it, I tried to find the issue, but for some reason was unable to do this. It seems it started to appear more often to me somehow after fish upgrade.

krobelus added a commit to fish-shell/fish-shell that referenced this issue Mar 8, 2025
@krobelus
Extend midnight commander workaround for when numlock/capslock is active
bd8d268 
Midnight Commander 4.8.33 knows how to read the CSI u encoding of ctrl-o
(which is the only key it reads while the shell is in control).  But it fails
to when numlock or capslock is active.  Let's disable the kitty keyboard
protocol again until mc indicates that this is fixed.

Closes #10640

The other issue talked about in that issue is an unrelated mc issue, see
MidnightCommander/mc#4597 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@zyv zyv

Labels
area: core Issues not related to a specific subsystem prio: low Minor problem or easily worked around
Milestone
4.8.33
Development

Successfully merging a pull request may close this issue.

Fix kitty keyboard protocol parsing of ctrl-o krobelus/midnight-commander
4 participants
@asl @zyv @krobelus @mc-butler