Skip to content

Clear task queue when Scheduler is shutdown#2881

Merged
ke-yu merged 4 commits intoDynamoDS:masterfrom
ke-yu:scheduler
Oct 29, 2014
Merged

Clear task queue when Scheduler is shutdown#2881
ke-yu merged 4 commits intoDynamoDS:masterfrom
ke-yu:scheduler

Conversation

@ke-yu
Copy link
Contributor

@ke-yu ke-yu commented Oct 29, 2014

Tasks on scheduler thread. In DynamoScheduler.Shutdown(), it notifies schedule thread to shutdown as well. Depending on scheduler thread's implementation, it is possible that although scheduler thread have been unavailable, but there are still some pending tasks in the queue. For example, RevitSchedulerThread uses Revit's idle event to process tasks, and if it is detached from idle event, tasks would never get a chance to run, so we need to make sure all tasks are cleared.

@benglin please.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like the documentation for this method to be updated as such:

/// Call this method to properly shutdown the scheduler and its associated
/// ISchedulerThread. This call will be blocked until the ISchedulerThread
/// terminates. Note that the implementation of ISchedulerThread may or may 
/// not choose to handle all the remaining AsyncTask in queue when shutdown 
/// happens -- DynamoScheduler ensures the tasks in queue are cleared when 
/// this call returns.

Which brings me to another question: there are various internal APIs on the DynamoScheduler: ProcessNextTask, ScheduleForExecution and Shutdown. It seems like there is nothing preventing these methods to be called after the shutdown has happened. What would you think, in terms of API design, that we should do in this case? I know for windows creation, typically we get exceptions like cannot call ShowWindow because the program is shutting down. Does this sound like something that needs to be built into DynamoScheduler APIs or is that overkill?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may refer to Dispatcher which has Shutdown()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overkill. I just read Dispatch.InvokeShutdown(), it aborts all pending tasks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good to know, but I guess you have misunderstood my original comment. My question was, do we need to throw an exception when some code calls DynamoScheduler.ProcessNextTask when DynamoScheduler has been shut down? Is that overkill?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I didn't. :-) I just read the implementation of Dispatcher, It doesn't prohibit you from calling Invoke() or BeginInvoke() after it is shutdown (at least from the code, but i'm not sure what happen at runtime). So probably it does nothing if you try to send more tasks to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway it is an interesting API design question, let's discuss that tomorrow :-)

@benglin
Copy link
Contributor

benglin commented Oct 29, 2014

Apart from the small comment above, this LGTM. Feel free to merge, and then cross merge.

ke-yu added a commit that referenced this pull request Oct 29, 2014
Clear task queue when Scheduler is shutdown
@ke-yu ke-yu merged commit a869d87 into DynamoDS:master Oct 29, 2014
@ke-yu ke-yu deleted the scheduler branch October 30, 2014 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants