load no longer works when used inside onecmd_plus_hooks

[tessebier]

As of 0.7.5 load uses cmdqueue, which means running load inside onecmd_plus_hooks no longer works as nothing processes the cmdqueue in that scenario. Ditto for any of the functions which call onecmd_plus_hooks like run().

If do_eos were to return self._STOP_AND_EXIT, and a self._cmdloop() were added back to the end of do_load(), it would work as it used to.

Not sure if such a change is acceptable and compatible with the intent of switching to use cmdqueue to begin with.

[alevy03]

@tessebier: I'm thinking about working on this issue. Can you elaborate on your use case and maybe give some minimal test case that replicates the problem you are having?

My understanding is that onecmd_plus_hooks doesn't call _cmd_loop in order to support using custom command loops or 3rd party frameworks. How are you using it? Do you call onecmd_plus_hooks in your own event loop? Or, do you invoke it as a one-off call?

There are a couple of different ways to solve this bug, but it would help to understand more about your use case so I don't implement a solution that isn't helpful for you.

[tessebier]

Thanks for responding and looking into it, much appreciated. I have a command tool that uses cmd2 with a CmdLoop() for standard cli type operations, primarily to execute a particular query command, call it "tql", and surrounding configuration commands to set up a suitable environment for tql. cache management, cache persistence, bunch of config/status command to on-process query results etc. with command history and all the trimmings. It works great! I also need to be able to use this in a one-shot mode however, where somebody just has the tool to run a single tql query and then exit back to the shell. Not least because this tool ends up being integrated into a splunk query in some incarnations in this sort of one-shot mode. To make this nice, I provide some switches to run 'load foo' and 'load bar' type cmd2 commands before and after the one-shot to pre-load and post-save the query environment. Which is done by one or more onecmd_plus_hooks before and after then main "do the tql thing" onecmd_plus_hook. Which all works smashingly, except for the load command, if I use 0.7.5 and older. So I don't call it in an event loop, but depending on tool-command-line-switches, I might do 0-2 calls prior to the 'main' query oneccmd_plus_hook, and one or two after. I also want to be able to use my Command() class as a an API for others to use, where similar "do this before/after" would also be either built-in to extra functions I add to my version of Command() or folks could call directly. In that instance one might well argue that the onecommand_plus_hook calls would be being called in some form of event-loop, as there is no control over how the 3rd party tool might want to drive this in order to get access to this tql environment and its results. However, I'd look at it more as a one-shot, when whoever needs to run a tql thing, kind of operation. That is as a sub-system sub-query type of operational mode, rather than being included into the main event loop 'every time around' sort of thing. Having onecommand_plus_hooks calling _cmdloop() would not be ideal. There is too much back/forth relationship between those two. If the requirements for 3rd party frameworks or custom commandloops permit do_load() to call _commandLoop() however, as I hinted at in my request, that would solve the matter of seemingly lost entries on the commandqueue in this case. I did put that in as stated to test, and while it fails a bunch of tests in the unit-test framework, for my purposes that would just work. But yes, I don't know enough of what else it might break doing that, otherwise I might in fact have done a PR myself :) Tom

…

On Sat, Sep 16, 2017 at 8:43 AM, alevy03 ***@***.***> wrote: @tessebier <https://github.com/tessebier>: I'm thinking about working on this issue. Can you elaborate on your use case and maybe give some minimal test case that replicates the problem you are having? My understanding is that onecmd_plus_hooks doesn't call _cmd_loop in order to support using custom command loops or 3rd party frameworks. How are you using it? Do you call onecmd_plus_hooks in your own event loop? Or, do you invoke it as a one-off call? There are a couple of different ways to solve this bug, but it would help to understand more about your use case so I don't implement a solution that isn't helpful for you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Aeflck4tA-NniRjC7A33jsGUewSh5y_Nks5siv2vgaJpZM4PYaNg> .

[alevy03]

I don't think we should add _cmdloop() to the end of do_load. _cmdloop sets readline configuration for the duration of the loop and then restores the original config when finished. _cmdloop stores those settings in member variables on self, and not in any kind of stack, so recursive calls are a no-no. If there was already an existing _cmdloop running, which executed the do_load, then the nested call to _cmdloop would stomp on the config and readline settings wouldn't be restored correctly. _cmdloop also cleans up the command queue and current script directory stack, so there will be similar issues with those.

We could make do_load aware of whether there is a running _cmdloop already and then only start _cmdloop if there is not. However, that doesn't cover the use case of a 3rd party command loop. In that case do_load would see there is no _cmdloop and start it running to drain the cmdqueue. After this, it will attempt to read more commands from stdin and, undoubtedly, interfere with or block whatever the 3rd party command loop was trying to do. This could be solved by requiring that the 3rd party loop set some sort of flag to make do_load aware that there is a loop already processing the cmdqueue, but I want to avoid introducing yet more states and flags and modes into the logic. I think that will complexify the implementation and unnecessarily couple things together. More succinctly: maintenance hazard.

Here is my proposal that looks to me like it will work for your use case:

1. Provide a convenience method that does the right thing for you.
   • Maybe called runcmds_plus_hooks.
   • Takes an iterable of command strings to run. It will call onecmd_plus_hooks on each of them, and also on any commands that get loaded into cmdqueue.
2. Document that onecmd_plus_hooks requires the cmdqueue to be drained.
   • Warn that the executed command may place commands in the cmdqueue. Currently only "load" does this, but maybe that could change in the future...
   • The caller must check the queue and pop commands off to give them to subsequent invocations of onecmd_plus_hooks if there is not already an event loop that is responsible for doing this.

Number 2 will document and preserve the existing behavior for onecmd_plus_hooks and allow finer grain control by custom event loops. Number 1 will allow you to do what you need with minimal fuss. All you need to do is create a list of all the operations you want to do (setup, body, cleanup, etc), and pass it to runcmds_plus_hooks. Example:

runcmds_plus_hooks(["load foo", "do the tql thing", "load bar"])

How does that sound? Any objections?

[tessebier]

Your reasoning seems sound, and I'm certainly not in a position to argue with it. I was indeed worried about the whole recursive and other potential side-effects aspect of placing any _cmdloop() invocations anywhere - I just don't know the code well enough. Adding a runcmds_plus_hooks that does it all seems a reasonable compromise, so no objections from me. Must have taken you a while to figure out - thank you for that effort. tom

…

On Wed, Sep 20, 2017 at 6:05 AM, alevy03 ***@***.***> wrote: I don't think we should add _cmdloop() to the end of do_load. _cmdloop sets readline configuration for the duration of the loop and then restores the original config when finished. _cmdloop stores those settings in member variables on self, and not in any kind of stack, so recursive calls are a no-no. If there was already an existing _cmdloop running, which executed the do_load, then the nested call to _cmdloop would stomp on the config and readline settings wouldn't be restored correctly. _cmdloop also cleans up the command queue and current script directory stack, so there will be similar issues with those. We could make do_load aware of whether there is a running _cmdloop already and then only start _cmdloop if there is not. However, that doesn't cover the use case of a 3rd party command loop. In that case do_load would see there is no _cmdloop and start it running to drain the cmdqueue. After this, it will attempt to read more commands from stdin and, undoubtedly, interfere with or block whatever the 3rd party command loop was trying to do. This could be solved by requiring that the 3rd party loop set some sort of flag to make do_load aware that there is a loop already processing the cmdqueue, but I want to avoid introducing yet more states and flags and modes into the logic. I think that will complexify the implementation and unnecessarily couple things together. More succinctly: maintenance hazard. Here is my proposal that looks to me like it will work for your use case: 1. Provide a convenience method that does the right thing for you. - Maybe called runcmds_plus_hooks. - Takes an iterable of command strings to run. It will call onecmd_plus_hooks on each of them, and also on any commands that get loaded into cmdqueue. 2. Document that onecmd_plus_hooks requires the cmdqueue to be drained. - Warn that the executed command may place commands in the cmdqueue. Currently only "load" does this, but maybe that could change in the future... - The caller must check the queue and pop commands off to give them to subsequent invocations of onecmd_plus_hooks if there is not already an event loop that is responsible for doing this. Number 2 will document and preserve the existing behavior for onecmd_plus_hooks and allow finer grain control by custom event loops. Number 1 will allow you to do what you need with minimal fuss. All you need to do is create a list of all the operations you want to do (setup, body, cleanup, etc), and pass it to runcmds_plus_hooks. Example: runcmds_plus_hooks(["load foo", "do the tql thing", "load bar"]) How does that sound? Any objections? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AeflcpqYLuoHhDiE1pOnjRRuRFuJZSToks5skB6KgaJpZM4PYaNg> .

[alevy03]

OK. I've made a PR that should fix it. @tessebier can you look at PR #227 and see if it works for you?