wizzomafizzo / mrext

Collection of extensions and utilities for the MiSTer system.
GNU General Public License v3.0
163 stars 11 forks source link

Improve write command #81

Closed wizzomafizzo closed 6 months ago

wizzomafizzo commented 6 months ago

@sigboe do you know if this will affect the nfcui.sh script at all?

sigboe commented 6 months ago

First of I wanted to say sorry for not answering quicker!

Second I don't see any issue this should have with production code

It does however conflict with this https://github.com/wizzomafizzo/mrext/pull/76 Where I in fact do the opposite. This doesn't mean we shouldn't be merged. In my hiatus I have done some testing, where I have developed a method of being able to outsource the animating of the screen to a function, that I think can be ran in a subshell(backgrounded). I am going to press the approve, I would have done that anyway as long as I would think it works with the production code.

Third, lets talk about the conventions for SIGTERM in unix programs.

SIGTERM or TERM or signal 15 is designed to tell the program to gracefully shutdown, clean up after it self, even not respond to the request, or ask the user questions, send notifications, or whatever you think a GUI program could do when you click the X button. This is the default way to quit programs command line and graphical and daemons.

SIGKILL or KILL or signal 9 this is designed to kill the application, don't do anything other than shutdown.


So my question is then what does this do in the middle of writing a card? Will it write half the card? will it corrupt the card? Will it finish writing?

In my PR #76 I stop responding to SIGTERM just before writing, and then the program naturally quits after that.

Anyway I will press approve on the PR, since I don't see this being a breaking change, I also want to know what happens when you TERM while writing?

Also, do you think TERM should restart the service? It seams to me you do that in L669? Anyway I won't use githubs comment function to have comments that need to be "resolved" before mergning

wizzomafizzo commented 6 months ago

@sigboe I agree with you completely. But I'm a little confused, am I using signal 9 somewhere that I'm not seeing?

I think it's an interesting point though about my use of signals to also trigger a restart. I'm co-opting the signal as a convenient way to receive a message that the write can be cancelled, not as it's intended to gracefully shutdown. Do you think we should use a different signal or different method entirely for requesting a write to be cancelled?

sigboe commented 6 months ago

@wizzomafizzo sorry again for taking my time to respond.

No I haven't seen anywhere where you are using signal 9 (SIGTERM) for something else. I have a pr open that does it, but I have a POC (on my local disk) for code where I won't need to do that. So feel free to merge as is!

My question is if you really want to quit the app mid write for signal 9

I suggest just finishing the write before quiting. This way it will quit gracefully, like how signal 9 is suggested to be used.

Signal 15 (SIGKILL) is the one to quit non-gracefully.

Also if you want a signal to restart the service, you may pick signal 9 or an unused signal number, or signal 9 is fine too. The only issue is that if you use signal 9 or 15 for this. And initiate this during when the user does something, the user can be interrupted, example if remote script is initiating signal 9 to nfc.sh while the user is manually reading or writing a tag. Then the the user will be interrupted.

https://www.man7.org/linux/man-pages/man7/signal.7.html

Keep mind, programs do indeed coopt these signal numbers for other tasks, but signal 9 and 15 are used to quit gracefully (9) or force quit (15) a program,

As i said, I think if you have started a write you may want to wait to quit until the write is finished. To quit gracefully.

And using signal 9 to double restarting the service, do it if you want, or coopt another signal to do it

sigboe commented 6 months ago

Keep in mind signal 9 and 15 works out of the box with golang,

So your chasing mainly just restarts the service using signal 9, which you may or may not want to do. That being said, you may use signal 9 for this, or you may use another signal.

Sorry if I am rambling here. Haven't gotten much sleep the last few days. I'll happily have a voice chat in discord if it helps

wizzomafizzo commented 6 months ago

I understand now! Thanks for explaining. I'll have a think about what to do

Hope you are going ok mate! Please don't feel obligated to reply on any schedule alright. I'm just writing notes. I appreciate you taking the time whenever works best for you 🙂