Community
Participate
Working Groups
I'm trying to integrate mylyn with my eclipse application; however, it never lets the job manager be idle. I know this, because when running PDE unit tests, I use this method on my setUp() to make sure that eclipse is ready to be used: public static void waitForJobs() throws Exception { while (!Job.getJobManager().isIdle()) { delay(1000); } } (delay waits for x ms, while letting the ui process events). This was worked quite well in a number of eclipse projects I've worked on, with multiple plugins. When running mylyn, the jobmanager is never idle. Running this through a debugger, it looks like the Flush Cache Job and the Task List Saver always run. Indeed, looking at CacheFlushJob, which is nested inside org.eclipse.mylyn.internal.tasks.ui.OfflineCachingStorage, it seems to be in a while(true) loop. The exact same is true for TaskListSaverJob, which is inside org.eclipse.mylyn.internal.tasks.ui.util.TaskListSaveManager I'm attaching patches for both files that fix these issues. Both Jobs are instantiated & scheduled only iff there are actions to execute. A lock is used to ensure that a job is not instantiated twice.
Created attachment 74911 [details] patch for the TasklistSaveManager
Created attachment 74913 [details] Patch for OffLineCachingStorage
(In reply to comment #1) > Created an attachment (id=74911) > patch for the TasklistSaveManager The general approach looks good, but this patch results in the tasklist not being saved. The saveTaskList method isn't pushing a save request under some conditions. Open a local task make some changes to the notes.... will not be saved. Ideally we'd also like to see unit test coverage of this.
Stefanos, are you planning to continue working on this issue?
Rob: if we don't hear back from Stefanos please review again post 2.1 to see if this can be applied. Stefanos: as per Eugene's comment, let us know if you are able to contribute a test and fix.
i'd be happy to continue on this; could you please let me know under which conditions the saveTaskList method isn't pushing a save request?
It's great you are able to continue working on this Stefanos. When you have a revised patch ready, append [patch] to the summary of this bug and it will get our attention much faster. To answer your question, the TaskListSaveManager.saveRequested() contract was that, if called, the task list would be saved upon next run of the BackgroundSaveTimer (held by the TaskListSaveManager). Your patch alters this slightly and jobs are only being added to the queue currently when saveContext=true is being passed to TaskListSaveManager.saveTaskList(...). So when actions take place that result in TaskListManager.containersChanged() being called, a save doesn't happen as part of the subsequent BackgroundSaveTimer save request.
Hi, My apologies for my delay in communicating. Things should hopefully improve. I may be have this wrong, but I'm slightly confused by the contract, at least as expressed in version 1.26 of TaskListSaveManager.java. According to that version, TaskListSaveManager.containersChanged() calls saveTaskList() with saveContext=false and async=true. Following the saveTaskList() logic, this bit will get executed: if (async) { if (saveContext) { for (AbstractTask task : taskListManager.getTaskList().getActiveTasks()) { taskListSaverJob.addTaskContext(task); } } taskListSaverJob.requestSave(); } since saveContext == false, the tasks to save will not end up in the queue in taskListSaverJob. However the job will still be scheduled, so this will run: while (!taskQueue.isEmpty()) { AbstractTask task = taskQueue.poll(); if (task != null) { contextManager.saveContext(task.getHandleIdentifier()); } } internalSaveTaskList(); The while loop will not be executed, as the taskQueue is empty. However, the internalSaveTaskList() method will be called. I'm sorry for being pedantic, but I found the class a bit difficult to follow. If I understand correctly, the only difference between my patch and the current head, is that if saveContext=false, then internalSaveTaskList() will not be run. Is this correct? If so, you'll have the new patch very soon :)
Stefanos: fyi Rob is away on vacation until Monday, and he'll look over this when he returns.
Sorry for the delay Stefanos. (In reply to comment #8) > I'm sorry for being pedantic, but I found the class a bit difficult to follow. This is tricky code and always takes me a minute or two to sort things out... which I guess is a good indicator that we need to refactor this. > If I understand correctly, the only difference between my patch and the current > head, is that if saveContext=false, then internalSaveTaskList() will not be run. > Is this correct? If so, you'll have the new patch very soon :) Yes, is my understanding. Where once the code resulted in a call to taskListSaverJob.requestSave(); after your patch the same code doesn't result in effectively requesting a save here when saveContext=false: public void saveTaskList(boolean saveContext, boolean async) { if (TasksUiPlugin.getDefault() != null && TasksUiPlugin.getDefault().isInitialized()) { TaskListManager taskListManager = TasksUiPlugin.getTaskListManager(); if (async) { if (saveContext) { for (AbstractTask task : taskListManager.getTaskList().getActiveTasks()) { taskQueue.add(task); } } jobLock.lock(); try { if(!taskQueue.isEmpty()) { TaskListSaverJob job = new TaskListSaverJob(taskQueue); job.schedule(); } } finally { jobLock.unlock(); }
Created attachment 79722 [details] second stab for the TasklistSaveManager will now schedule execution of internalSaveTasklist() even if the taskQueue is empty
Reviewed patch, here are my notes: * calling internalSaveTaskList() directly from within if(async) can't be done since internalSaveTaskList() is synchronous and could result in blocked ui if called from ui thread * I think a fundamental problem here may also be that running of the save is dependent on the queue being populated. The queue is only populated a) when true is passed for save context, and b) when there is an active task. A better way to look at this is perhaps to rename the queue to taskContextQueue since this is simply the queue of task contexts to save -along- with the saving of the tasklist itself. So when save is called a) the active task should be added to the queue if context=true, b) a request to save the tasklist should be registered (we were using a boolean). Then it becomes the job of the BackgroundSaveTimer to actually invoke the save. For an immediate save and flush of the context save queue, save called with asynch = false. How does this sound?
Any chance this will looked into ? Never idle taskmanager is a pain when trying to do testing...
Rob, this has been addressed with the recent changes to the externalization jobs, right?
Fixed.
