Page MenuHomePhabricator | MITK

The BlueBerry jobs API is unuseable
Open, NormalPublic

Description

The berry::Job implementation uses an internal class as base (berry::InternalJob). The

void AddJobChangeListener(IJobChangeListener* listener);

method, declared in berry::Job, is actually implemented in berry::InternalJob. The proxy implementation in berry::Job is missing so that it is impossible to add IJobChangeListener in to a job, because berry::InternalJob is private.

All of that is located in the org.blueberry.core.jobs plugin.

Details

Differential Revisions
Restricted Differential Revision
Restricted Differential Revision

Event Timeline

aurelien created this task.Aug 5 2016, 2:09 PM

The fix is very simple but without it, the Job API is quite unusable.

diff --git a/Plugins/org.blueberry.core.jobs/src/berryJob.cpp b/Plugins/org.blueberry.core.jobs/src/berryJob.cpp
index db8b0da..63a6c83 100644
--- a/Plugins/org.blueberry.core.jobs/src/berryJob.cpp
+++ b/Plugins/org.blueberry.core.jobs/src/berryJob.cpp
@@ -125,6 +125,11 @@ void Job::RemoveJobChangeListener(IJobChangeListener* listener)
   InternalJob::RemoveJobChangeListener(listener);
 }

+void Job::AddJobChangeListener(IJobChangeListener* listener)
+{
+  InternalJob::AddJobChangeListener(listener);
+}
+
 void Job::Schedule()
 {
   Poco::Timestamp::TimeDiff tmpNoDelay = 0;
aurelien renamed this task from Cannot add a berry::IJobChangeListener in a berry::Job to The BlueBerry jobs API is unuseable.Aug 5 2016, 2:20 PM
zimmerm added a subscriber: zimmerm.Aug 5 2016, 2:38 PM

There are some other errors in the API with the smart pointers. I often have my Job deleted before execution.
I'm investigating on it.

nolden added a subscriber: zelzer.Aug 8 2016, 4:05 PM
nolden added a subscriber: nolden.

In some functions, 'this' (as instance of InternalJob) is wrapped into berry::SmartPointer.
When the function ends, then the pointer counter falls to 0 and the object is destroyed.
From my point of view we can use pointer everywhere, and the Job pointer can be deleted when finished,
if no rescheduled, when all listeners has been called.

Ex. in InternalJob::Schedule() around line 344 :

void InternalJob::Schedule(Poco::Timestamp::TimeDiff delay)
{
  if (ShouldSchedule())
  {
    // previously : ptr_manager->Schedule(InternalJob::Pointer(this), delay, false);

    // do not delete 'this' when Schedule() ends
    InternalJob::Pointer ptr(this);
    ptr_manager->Schedule(ptr, delay, false);
    ptr->UnRegister(false);

  }

}

In the other hand, if i create a job using a berry::SmartPointer, I no longer have the early deletion problem thanks to internal ref counter. In that case, I can launch the job only one time : the second time the job is fired, the first one has not been destroyed and i face an UI freeze (and have to kill the application)

After some more learning in Berry and smart pointer, I'm now quite done. No more freeze and clean Job subclassing.

I attached a patch i applied to have the following scenario to work :

  • create a custom job which inherits berry::Job
  • add a listener to the job, to be able to process its result
  • set the job priority as SHORT
  • set job as 'user'
  • schedule the job
  • job executed
  • listener code called
  • job pointer deleted

Modified code :

  • berryJobQueue.cpp l.141, berryJobManager.cpp l.1118 : I added some defensive code
  • berryJobManager.cpp l.874 : remove done job rather than reschedule it (allow to call its destructor after listener call dispatch)
  • berryWorker.cpp l.53 : catch the actual return IStatus from the Job's 'Run()' method

Sometimes when the application gets the focus, i still have a crash in JobQueue::Peek(), in the smart pointer debugging related code :

liborg_blueberry_core_runtime.dll!QHash<unsigned int,QList<unsigned int> >::findNode(const unsigned int & akey, unsigned int * ahp) Line 958	
 	liborg_blueberry_core_runtime.dll!QHash<unsigned int,QList<unsigned int> >::operator[](const unsigned int & akey) Line 792	
>	liborg_blueberry_core_runtime.dll!berry::DebugUtil::UnregisterSmartPointer(unsigned int smartPointerId, const berry::Object * objectPointer) Line 360
 	liborg_blueberry_core_jobs.dll!berry::SmartPointer<berry::InternalJob>::DebugRemoveSmartPointer() Line 412	
 	liborg_blueberry_core_jobs.dll!berry::SmartPointer<berry::InternalJob>::~SmartPointer<berry::InternalJob>() Line 110
 	liborg_blueberry_core_jobs.dll!berry::JobQueue::Peek() Line 143

It looks like someone tries to delete the 'dummy' object.

Thank you @aurelien. It's really awesome that you're working on the issue. You can also use Differential for your diff:
https://phabricator.mitk.org/differential/diff/create/

kislinsk triaged this task as High priority.Aug 9 2016, 9:05 AM
kislinsk edited projects, added MITK (2016-11); removed MITK.
kislinsk added a revision: Restricted Differential Revision.Aug 9 2016, 3:04 PM
aurelien added a comment.EditedAug 9 2016, 3:07 PM

There is still problems with scheduling, but I have to go on other tasks for the moment. The API is useable even there are still problems with scheduling. I should come back on this ticket once we get the plugin test infrastructure back to work.

Cool, thank you again. I just renamed the revision and edited the related objects instead. So this task and the revision are linked in the system.

no problem. Thanks for the link i'm not yet aware of a lot of Phabricator features

No worries, we're still learning as well.

zelzer added a comment.Aug 9 2016, 4:03 PM

I thought I'll give you some historical background about the Jobs API.

The implementation was part of a master thesis I supervised a few years ago. It was during the early stage of the whole BlueBerry project. The unit tests (which should still be in the repository) were working back than, but might have been broken in the mean time unfortunately. The current state of our plugin unit tests is another story unfortunately.

Of course there have been parts of the Jobs API that could not be finished in the scope of the masters thesis. If I remember correctly, the job listeners was part of it. Another large missing part to make it really useful for end-users is UI support. E.g. listing running Jobs in a "Jobs View", showing their progress, and the ability to cancel long running tasks.

Thanks i better see. The implementation is good and follows the Eclipse 3 Job API. The job listeners should work, and the "job view" can be plugged using a 'ProgressProvider' concept. I think we will work on that part soon.
We also have the plugin tests quite working (the tested plugin is still not loaded, but whole code now compiles). It should be done within next month -i hope- because we really need testing :)
Existing test code for job api would be then plugged back.

kislinsk lowered the priority of this task from High to Normal.Aug 11 2016, 7:12 AM
kislinsk edited projects, added MITK; removed MITK (2016-11).
Truite added a revision: Restricted Differential Revision.Sep 28 2017, 11:44 AM
Truite added a subscriber: Truite.EditedSep 28 2017, 12:57 PM

@aurelien @kislinsk We manage to have a more stable API now (see D72 ) whith a bunch of tests We may need to challenge it a little bit more in real condition though! We'll keep you posted.