rovellipaolo / NinjaDroid

Ninja Reverse Engineering on Android APK packages
GNU General Public License v3.0
268 stars 49 forks source link

JobExectutor isn't asynchronous #15

Closed iantruslove closed 4 years ago

iantruslove commented 5 years ago

I'm not quite sure what to do about this one...

The problem is that none of the async stuff is actually async because none of the ThreadPoolExecutor invocations are with callables, they're all with the result of actually calling the "work" functions.

For example, here:

os.system(command) is run and its result captured before being sent to the executor's submit method.

To actually call this and have it run asynchronously, you could do it e.g. with a little tweak to JobExecutor

class JobExecutor:
    # ...
    def submit(self, fn, *args, **kwargs) -> Future:
        return self.executor.submit(fn, *args, **kwargs)

then work to be done in the threadpool gets called by passing in a callable, plus any args, e.g. using the earlier example:

class LaunchDex2Jar(UseCase):
    # ...
    def execute(self) -> Future:
        # ...
        command = LaunchDex2Jar.DEX2JAR + " -f " + self.input_filepath + \
                  " -o " + self.output_directory + "/" + jarfile

        return self.executor.submit(os.system, command)

But here's the tricky bit... If you do this, there are new issues, most obviously (1) waiting for all the futures in a group to finish, and (2) the very real possibility of race conditions.

I'm not sure how you'd like to proceed with this. I'd be tempted to simply rip out all the executor stuff because it's effectively just dead code not providing any benefit. Happy to lend a hand if you have any thoughts...

rovellipaolo commented 4 years ago

You are totally right. However, I'm still not totally sure on what to do with this.

From one side, yes, we could execute everything synchronously and get rid of the JobExecutor. Most of the jobs have dependencies with another one and must be executed in a chain-fashion one after the other anyway (e.g. see ExtractApkEntries).

However, some of the jobs could actually be executed in parallel (e.g. GetApkInfoInHtml and GetApkInfoInJson) and, by using Future, we could chain them easily.

rovellipaolo commented 4 years ago

Finally, I had some spare time and I've decided to remove the JobExecutor and execute the use cases synchronously: 1ebf316f70b0a299cfde7a1d3d559700ca444edd