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

TUI exits on gpg command abort #7

Open
orhun opened this issue May 29, 2021 · 9 comments
Open

TUI exits on gpg command abort #7

orhun opened this issue May 29, 2021 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@orhun
Copy link
Owner

orhun commented May 29, 2021

Describe the bug

rec

To Reproduce

Steps to reproduce the behavior:

  1. Run with any arguments
  2. Press g (generate key)
  3. When gpg is waiting for input, press Ctrl-C
  4. See that TUI is not resuming and exited

Expected behavior

gpg-tui should continue working after aborting a gpg command.

System Information

  • OS: [e.g. Arch Linux x86_64 5.12.7-arch1-1]
  • Rust Version: [e.g. 1.54.0-nightly]
  • GPGME version: [e.g 1.15.1]
  • GPGME engine version: [e.g. 2.2.27]
  • Project Version: [e.g. 0.1.3]

Additional context

If you press e and see gpg --edit-key command is ran, you can abort gpg and resume TUI via typing q or quit.

@orhun orhun added the bug Something isn't working label May 29, 2021
@orhun orhun self-assigned this May 29, 2021
@Tamschi
Copy link

Tamschi commented Oct 1, 2021

Hi from the Hacktoberfest discord 🖖!

This one looks interesting. I've used a signal handler before (to gracefully shut down a container), but haven't forwarded a SIGINT yet.

Rust doesn't seem to have a built-in way to send signals, other than SIGKILL which seems inappropriate. I'd have to do a bit of reading there, since I'm usually stuck on Windows, but this is an area I should learn more about anyway, so that works out nicely.

I'd enable a signal handler only while the child process runs and otherwise default to the normal behaviour of aborting the process.
The fix seems mostly straightforward, but I'd likely need your input on how to treat race conditions at the edges of the redirect window. I'd go for a simple solution first before getting back to you about that, though.


I'm a bit busy next week, but if the above sounds reasonable, you can assign me and I should have it fixed by the end of next weekend.

(That's how it works, right? My crates aren't popular, so I haven't really gotten contributions on them yet 😅)

@orhun
Copy link
Owner Author

orhun commented Oct 1, 2021

Hello @Tamschi, sure thing. This is actually a little bit critical since seeing gpg-tui terminated on these cases might get annoying after some point 🐻 So feel free to look into it and share your findings. It would be really awesome if you come up with a solution that fixes it. Let me know if there is something that I can help with :)
Thank you!

@Tamschi
Copy link

Tamschi commented Oct 1, 2021

Sure thing! There's actually one question I have before starting:

I see that you've combined the branches for commands that launch gpg.
Would you like to have the handler active for all of them or only for interactive ones?

@orhun
Copy link
Owner Author

orhun commented Oct 1, 2021

I see that you've combined the branches for commands that launch gpg. Would you like to have the handler active for all of them or only for interactive ones?

I think having the handler only for the interactive ones would be good. Non-interactive (single shot) commands does not have this problem afterall.

@Tamschi
Copy link

Tamschi commented Oct 1, 2021

I see that you've combined the branches for commands that launch gpg. Would you like to have the handler active for all of them or only for interactive ones?

I think having the handler only for the interactive ones would be good. Non-interactive (single shot) commands does not have this problem afterall.

Okay, in that case I can simply use the default behaviour in edge cases and only consider SIGINTs where the user is reacting to an interactive prompt. I think that makes for the best trade-off between UX and complexity.

@Tamschi
Copy link

Tamschi commented Oct 3, 2021

A small progress update, since I found some time yesterday but likely won't continue for a few days now:

It was easier to try this in isolation (and turned out to be really easy to encapsulate in the end), so I spun up a library project that I'll (also) publish for other crates to use. You can see that here, it's working on Unix so far (though I can't quite easily test it on Mac; maybe if I learn a bit more about doing CI there later on): https://github.com/Tamschi/clack#readme
Please excuse the unpolished code at this stage, it's in the "creative" phase 😅

The main TODOs (polish aside) are:

  • Switching to libc directly (from nix), since I'm barely using the abstractions really, and it would be slightly safer if I'm not mistaken.
  • Windows support, which is possible but more restrictive. I may have to spin up a handler thread there.
  • Better error handling/ignoring errors where appropriate

Since I'm making a library, there are two ways I could implement the fix for gpg-tui:

  1. Once I have something more complete, I could quickly make a simplified version for use specifically in this program.

    I estimate that it would be about as much code as I have in unix.rs right now, for each OS family but with no further glue or abstraction, and much less validation necessary than in my spin-off project.

  2. I could add a dependency on my package once that's ready for release.

    Unfortunately the name is taken by a "placeholder" crate on Crates, but I'm contacting its owner to see if I can use it. Otherwise I'll have to think of something else. His project is more deserving of the name, so I thought up a different one.

    I put in a little bump in the API to make it harder to misuse accidentally, so I think I'd have to add about 2-15 lines to launcher.rs where relevant Children are .spawn()ed.

    Please check my Semver policy in the README to see if it's appropriate for this project.
    Note that I can only really uphold it when dependencies are resolved via cargo +nightly update -Z minimal-versions. (I haven't gotten around to adding a note in that regard yet, but will do so before release.)

Feel free to answer this later when my code is more complete. I'll let you know when I'm ready to start work on the PR itself.

@Tamschi
Copy link

Tamschi commented Oct 24, 2021

You may have noticed, but I haven't updated my repository in a while. A few things happened that mean I'm having to prioritise more what I spend my energy on for the time being.

I'm sorry to say, but as it stands right now, it's unlikely I'll be able to submit a fix in a time frame where staying assigned to the issue would make sense. (I.e. I have to suspend this project indefinitely. It would be better to put the issue up for grabs again.)

@orhun
Copy link
Owner Author

orhun commented Oct 24, 2021

You may have noticed, but I haven't updated my repository in a while. A few things happened that mean I'm having to prioritise more what I spend my energy on for the time being.

I'm sorry to say, but as it stands right now, it's unlikely I'll be able to submit a fix in a time frame where staying assigned to the issue would make sense. (I.e. I have to suspend this project indefinitely. It would be better to put the issue up for grabs again.)

Ah, no problem, I'm sorry to hear that. I hope everything will get better for you soon 🙏🏼

Feel free to tackle this issue again if it is not resolved in the future 👍🏼

@sergeevabc
Copy link

Any chance to try TUI binary on Windows 7 x64?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants