Making the CLI usable #1034

New Issue

2017-07-24T10:24:40+02:00

Lvl4Sword commented

2017-07-24 10:24:40 +02:00

(Migrated from github.com)

I'm working on a new, better version of the CLI here. There are some issues I've bumped into and I can't fix them without changes in Bitmessage itself.

The one BIG issue I'm having is regardless of the signal I send to bitmessagemain ( SIGTERM or SIGKILL ) within the CLI, it doesn't close. So, trying to figure out why that's the case..

doCleanShutdown does nothing for the CLI, and this should be looked into. ( https://github.com/Bitmessage/PyBitmessage/blob/master/src/shutdown.py#L19 )
os.fork() being used, when it shouldn't be in any code whatsoever. It also doesn't work on Windows, and makes shutting down Bitmessage in the CLI via subprocess impossible. ( https://github.com/Bitmessage/PyBitmessage/blob/master/src/bitmessagemain.py#L278 )
Instances of os.exit() rather than sys.exit() in certain areas ( https://github.com/Bitmessage/PyBitmessage/blob/master/src/shutdown.py#L72 )
Catching signals through setSignalHandler that uses helper_generic BREAKS the ability to use .terminate() in subprocess

There may be other issues as well, but these are the big pieces I've found.

I'm working on a new, better version of the CLI [here](https://github.com/Lvl4Sword/PyBitmessage-CLI/). There are some issues I've bumped into and I can't fix them without changes in Bitmessage itself. The one BIG issue I'm having is regardless of the signal I send to bitmessagemain ( SIGTERM or SIGKILL ) within the CLI, it doesn't close. So, trying to figure out why that's the case.. - [ ] doCleanShutdown does nothing for the CLI, and this should be looked into. ( https://github.com/Bitmessage/PyBitmessage/blob/master/src/shutdown.py#L19 ) - [ ] os.fork() being used, when it shouldn't be in any code whatsoever. It also doesn't work on Windows, and makes shutting down Bitmessage in the CLI via subprocess impossible. ( https://github.com/Bitmessage/PyBitmessage/blob/master/src/bitmessagemain.py#L278 ) - [ ] Instances of os.exit() rather than sys.exit() in certain areas ( https://github.com/Bitmessage/PyBitmessage/blob/master/src/shutdown.py#L72 ) - [ ] Catching signals through setSignalHandler that uses helper_generic BREAKS the ability to use .terminate() in subprocess There may be other issues as well, but these are the big pieces I've found.

👍 1

PeterSurda commented

2017-07-27 11:13:11 +02:00

(Migrated from github.com)

doCleanShutdown should be fixed, you're right
I'm still not convinced to get rid of os.fork, but I'll change it for windows as that's not implemented and other people also reported it
I'll review the os.exit, that seems a reasonable request, however in some cases I think it's preferable (when you run out of disk space, for example).

- doCleanShutdown should be fixed, you're right - I'm still not convinced to get rid of os.fork, but I'll change it for windows as that's not implemented and other people also reported it - I'll review the os.exit, that seems a reasonable request, however in some cases I think it's preferable (when you run out of disk space, for example).

Lvl4Sword commented

2017-07-27 21:46:04 +02:00

(Migrated from github.com)

Do you understand as to why os.fork was put in to begin with? The docs ( https://docs.python.org/2/library/os.html ) state:

Exit the process with status n, without calling cleanup handlers, flushing stdio buffers, etc.
Availability: Unix, Windows.

Note
The standard way to exit is sys.exit(n). _exit() should normally only be used in the child process after a fork().

which means that we're not getting a clean shutdown like sys.exit ( https://docs.python.org/2/library/sys.html#sys.exit )

That then makes us need to address the /need/ ( or lack there of ) for os.fork.

You should never call os.fork yourself. but also, self-daemonization is an outdated idea that isn't relevant anymore ( Credit goes to _habnabit of #python )
- With this being the case, what's the suggested route? Do nothing and let your process manager handle it. i.e., don't use os.fork
It's only available for unix ( as from https://docs.python.org/2/library/os.html#os.fork ), so that's an issue as well. How is it suppose to work on Windows? Why have two separate ways of doing the same thing when we can have one through subprocess ( which works ).

Do you understand as to why os.fork was put in to begin with? The docs ( https://docs.python.org/2/library/os.html ) state: Exit the process with status n, without calling cleanup handlers, flushing stdio buffers, etc. Availability: Unix, Windows. Note The standard way to exit is sys.exit(n). _exit() should normally only be used in the child process after a fork(). which means that we're not getting a clean shutdown like sys.exit ( https://docs.python.org/2/library/sys.html#sys.exit ) That then makes us need to address the /need/ ( or lack there of ) for os.fork. - You should never call os.fork yourself. but also, self-daemonization is an outdated idea that isn't relevant anymore ( Credit goes to _habnabit of #python ) - With this being the case, what's the suggested route? Do nothing and let your process manager handle it. i.e., don't use os.fork - It's only available for unix ( as from https://docs.python.org/2/library/os.html#os.fork ), so that's an issue as well. How is it suppose to work on Windows? Why have two separate ways of doing the same thing when we can have one through subprocess ( which works ).

👍 1

PeterSurda commented

2017-07-28 08:46:40 +02:00

(Migrated from github.com)

Regarding os.exit, if you run out of disk space and/or there are errors with disk reading/writing, and this triggers cleanup procedures, these may also fail for the same reason. That is why in such a case I'd prefer a hard exit, but that's basically the only exception.

Regarding daemonizing, that isn't merely to satisfy the system tools, it also has security benefits, such as closing filehandles, changing permissions, releasing memory and so on. Systemd or upstart can handle programs that don't daemonize, but some people may prefer more traditional tools like the BSD init, which is easier to do with a traditional daemon. Now, it's true that there needs to be an additional check for Windows. Moreover the code was broken in 0.6.2 (but that should be fixed now). But that isn't such a big deal, I just checked and on windows, os.fork() throws an AttributeError, so I'll just wrap an exception handler around it. If that still isn't enough, I can add a config option to make fork optional.

Regarding os.exit, if you run out of disk space and/or there are errors with disk reading/writing, and this triggers cleanup procedures, these may also fail for the same reason. That is why in such a case I'd prefer a hard exit, but that's basically the only exception. Regarding daemonizing, that isn't merely to satisfy the system tools, it also has security benefits, such as closing filehandles, changing permissions, releasing memory and so on. Systemd or upstart can handle programs that don't daemonize, but some people may prefer more traditional tools like the BSD init, which is easier to do with a traditional daemon. Now, it's true that there needs to be an additional check for Windows. Moreover the code was broken in 0.6.2 (but that should be fixed now). But that isn't such a big deal, I just checked and on windows, os.fork() throws an AttributeError, so I'll just wrap an exception handler around it. If that still isn't enough, I can add a config option to make fork optional.

PeterSurda commented

2017-07-28 08:56:42 +02:00

(Migrated from github.com)

I haven't tried it, so if you're still having problems, let me know.

Lvl4Sword commented

2017-07-30 00:40:11 +02:00

(Migrated from github.com)

Still having problems. SIGTERM does nothing for bitmessagemain, but works for every other program I use subprocess' Popen, and SIGTERM on.

Lvl4Sword commented

2017-08-02 20:59:49 +02:00

(Migrated from github.com)

Looks to be working on Linux now, can you test for Windows/Mac/BSD?

Lvl4Sword commented

2017-08-15 02:50:27 +02:00

(Migrated from github.com)

Added:
Catching signals through setSignalHandler that uses helper_generic BREAKS the ability to use .terminate() in subprocess

Why exactly are signals being caught anyway? Surely there's a better way to approach this...

Added: Catching signals through setSignalHandler that uses helper_generic BREAKS the ability to use .terminate() in subprocess Why exactly are signals being caught anyway? Surely there's a better way to approach this...

This repo is archived. You cannot comment on issues.