1. Juan Pablo Caram Avatar Juan Pablo Caram
  2. Untitled project
  3. FlatCAM
  4. Issues

Implement execution blocking in shell command

Issue #196 new
Juan Pablo Caram repo owner created an issue

Consider the following in TCL:

set x [myfunction]

We want myfunction to return some value that we can store in x.

The problem arises when myfunction needs to do anything in the background (thread). It will complete and return while the background task is still running.

The solution is for myfunction to wait until all tasks are complete and provide a return value based on their results. But this is not as simple as checking for some variable value, because blocking execution blocks the main event loop, preventing signals from threads to be handled.

This is one approach that I've found: http://www.gulon.co.uk/2014/06/03/waiting-for-a-signal/

This problem is related but different to #145.

Comments (28)

  2. Juan Pablo Caram reporter

    @sopak please take a look at this issue and the commit mentioned above. The solution was quite simple. I just want you to be aware of it so whatever you implement is compatible with it, and that you use it if you need to.

  3. Juan Pablo Caram reporter

    I'm not closing yet until we do more tests, but this is how the solution works for now:

    def mytest:
  with wait_signal(self.new_object_available, 5000):
    some_threaded_task();
  return get_new_data();

    mytest is any shell command. It can run some task some_threaded_task that runs in the background but will not continue until a signal, self.new_object_available in this example, is emitted.

  6. Kamil Sopko

    Hi i test it, it works, but there is major problem within this approach, I never gets exception if anything went wrong inside threaded task.

    Because we speak mainly about FlatCamOBJ.generatecncjob, I implement threaded and non threaded execution. It solves all my problems with promises, there are other places where you are using promises and threads, but it does not affect shell.

    I understand the fact, if someone will manually generate cncjob I can get errors within shell, but it will be very rarely used with export_gcode together. Also mixing shell and manual commands will be very rare.

    I left it implemented within my branch tcl-errors:

    Go to tclCommands ...  TclCommandCncjob and  into class definition put 
            class TclCommandCncjob(TclCommand.TclCommandSignaled): 
        also change  
            obj.generatecncjob(use_thread = False, **args)
        to
            obj.generatecncjob(use_thread = True, **args)

    TclCommand.TclCommandSignaled is implemented in TclCommand module for now

    Last changes can be found here 9806386

  7. Juan Pablo Caram reporter

    Also mixing shell and manual commands will be very rare.

    That is not the point. It's about running thing in threads so the GUI continues to be operative. If you run a long command, from the shell or from the GUI, the GUI will freeze. If you run a command that takes 1 minute for example, you cannot even pan/zoom the plot during this time. Even worse, most operating systems will think that your program froze and show a warning.

    Because we speak mainly about FlatCamOBJ.generatecncjob ...

    This is just for now. Eventually, we want to do most tasks in their own threads because they are becoming more and more complicated every time. And of course, new functionality will be implemented that we will most likely want to run in the background.

    It solves all my problems ...

    I would rather we focused on solving the more general problems first.

    I never gets exception if anything went wrong inside threaded task.

    This post: http://www.gulon.co.uk/2014/06/03/waiting-for-a-signal/ seems to have a more elaborate solution that takes care of exceptions better. Could you post a short explanation of how to reproduce the problem that you saw so I can take a look at how to solve this?

    I implement threaded and non threaded execution ...

    Non-threaded execution is more of a workaround than a solution. Eventually, many things will have to run in separate threads and we don't want all developers having to create their own modified versions of the core functions just to have their shell commands work properly. Furthermore, as the program grows it will become very hard to determine what is running in the main or on separate threads.

    I know you've been writing a lot of code so far. If you have small things that can be pulled in please do so. It gets really hard for me to review huge changes. Thanks.

  8. Kamil Sopko

    I did think about this threads and it will be much better to do it on exec_command level and tasks should have to be possible to run without threads for easy debugging/testing, thread should start and be handlet when they are needed.

    With your solution anything happen within thread is silently ignored, no dialog for UI and no data for console, it can be catched but it will become only harder.

    Ideal would be to have main UI thread as is and one worker thread with queue, in Java we use https://docs.oracle.com/javase/7/docs/api/java/lang/Runnable.html whch is simple interface and then thread pool(can have one thread) which takes tasks from queue. This can be easy for shell TCL , but hard for some interactive tasks.

    changes what I created are not too big. I did only very little changes in current code, implementation of shell commands is separated and only overwrite specific commands, but leave there old implementation, it can be turn off/on in tclCommands/init.py by commenting out commands+ some PEP8 cleanups are there but its minor.

    I guess we have to have skype call maybe.

  9. Kamil Sopko

    I want rewrite all shell commands to new style and complete tests, but I am not sure if you are ok with that OOP style

  10. Juan Pablo Caram reporter

    Absolutely. I really liked the structure you were using.

    Just make sure to check with me if you think anything you are changing might cause any conflicts.

  12. Kamil Sopko

    OK then I will do 3 steps at once and i am sorry for that, it will be probably huge pull request

    1] implement that all exec_command calls will be executed in separated thread (all inside should be non threaded)

    2] reimplement all shell commands and clean nonsence like return OK etc (old commands will stay in place, for now)

    3] implement test for all shell commands

  13. Juan Pablo Caram reporter

    @sopak I added a new test function mytest2 to look at the exception handling problem. Run like mytest2 5000 where the parameter is the timeout in ms.

    What I'm doing is this:

            def mytest2(*args):
            to = int(args[0])

            try:
                for rec in self.recent:
                    if rec['kind'] == 'gerber':
                        self.open_gerber(str(rec['filename']))
                        break

                basename = self.collection.get_names()[0]
                isolate(basename, '-passes', '10', '-combine', '1')
                iso = self.collection.get_by_name(basename + "_iso")

                with wait_signal(self.new_object_available, to):
                    1/0  # Force exception
                    iso.generatecncjob()

                return str(self.collection.get_names())

            except Exception as e:
                return str(e)

    When I run it I get:

    > mytest2 1000
integer division or modulo by zero

    which is exactly what's expected. Can you help me construct a case to reproduce the problem that you are seeing?

  14. Kamil Sopko

    yes, but thats becouse error happen in main thread, what I was trying to solve is errors within obj.generatecncjob(use_thread = True, **args) calls.

    You are starting thread inside generatecncjob and if there is error, it is not catched and command is waiting until timeouted.

    Therefor I will implement solution, give me 10 minutes.

  15. Juan Pablo Caram reporter

    1] implement that all exec_command calls will be executed in separated thread (all inside should be non threaded)

    Please wait!!!

    This is a totally different approach that we have not discussed.

  16. Kamil Sopko

    your solution

    1] does not work

    2] depends on shell command implementator, or someone who in future will implement threads inside long tasks

    it should be done one one place and only FlatCamOBJ.generatecncjob have this threded behaviour for now

    it should have entry point from shell which will execute it in worker thread and probably also some wraper from UI

  17. Juan Pablo Caram reporter

    The program will still need to have multiple things done in separate threads. You cannot change the whole structure and have everything run in the foreground just to make it easier to run the shell commands.

    You are starting thread inside generatecncjob and if there is error, it is not catched and command is waiting until timeouted.

    Well, then this needs to be fixed, and not change the whole structure.

    1] implement that all exec_command calls will be executed in separated thread (all inside should be non threaded)

    You are going to run into all the same problems. Catching the exceptions in the threads, and still needs the blocking to be able to return data and reuse it in TCL.

    If you want you can do the OO approach (I trully like it and you should move ahead) and we figure out the threading/blocking later. But we need to agree on all these items.

    If you disagree about the blocking approach that I've been proposing, assuming that I/we solve the exception handling problem, I need to know why. My argument is for scalability and ensuring the whole project can continue to grow without adding severe constraints to the structure.

  18. Kamil Sopko

    Let me show you implementation, it will work, give me some time, I am working on it but have some minor issues

  19. Juan Pablo Caram reporter

    I believe you it will work. But FlatCAM is not just the shell and I need to make sure that the whole app functions nicely and can keep growing efficiently. That's why we have to agree on major structural changes.

    Go ahead and submit the pull request, but be aware that I need more of a teamwork attitude from you.

  21. Juan Pablo Caram reporter

    Current status. This shell command correctly raises an exception in TCL when an exception is raised in a background thread (in Python):

    def mytest4(*args):
    to = int(args[0])

    def sometask(*args):
        time.sleep(2)
        1/0  # Force exception
        self.inform.emit("mytest4")

    with wait_signal(self.inform, to):
        self.worker_task.emit({'fcn': sometask, 'params': []})

    return "mytest3 done"

    In TCL:

    > mytest4 3000
ERROR: integer division or modulo by zero
    while executing
"mytest4 3000"

    Note: This still only returns after the timeout expires. The inly thing pending is to emit a signal from the threads whenever there is an exception in them. This is a trivial addition that I will include soon.

  22. Kamil Sopko

    ok I understand, but still not agree. Main reason why I advocate other solution is to follow rule "implement once, use many times",

    with your design you should implement and handle it always inside command, I am trying to reuse it and set rule all shell commands will run in worker thread. It will keep commands clean and easy to understand. Similar behavior would be probably good to use from UI, but that is not my part .

    I will try show you my approach tomorrow, then we can discuss it more.

  24. Kamil Sopko

    I merge today your work into mine ;) and you change it again, will comit it anyway need to clean table

  25. Juan Pablo Caram reporter

    I can move the wait_signal function to a different place. Then there wouldn't be a problem merging. Let me know.

  26. Kamil Sopko

    I have some trouble with executing tests, In real environment it works.

    I implement solution which is based on your signals, but during tests it hags randomly when more tests is executed in one TestCase.

    I commited it now to my tcl-errors branche, with test problems, please ignore them for now.

    if you can have look (e96ee1a)

  27. Kamil Sopko

    First idea was to run TCL in different thread, but it cause huge amount of issues, therefor only each individual shell command is executed as signaled command (excluding new).

    I dont wait for new_object_available but shell_command_finished which is emited after real body of command:

        def execute_call(self, args, unnamed_args):

        self.output = self.execute(args, unnamed_args)
        self.app.shell_command_finished.emit(self)
  29. Log in to comment
Assignee
–
Type
bug
Priority
major
Status
new
Votes
0
Watchers
1
Jira: the preferred issue tracker for Bitbucket. Join the team!