Review of solution for Cancelled Tasks
steve.x.northover at oracle.com
steve.x.northover at oracle.com
Wed Jan 4 12:05:06 PST 2012
The API is ugly and I'm not sure is does much other than stop tasks that
are canceled from reporting progress. But I think that was the issue we
were wondering about?
If the only way to report progress is to (bogusly) "check for cancel,
provide a status message, provide an amount worked" all at the same
time, then I supposed you could still keep going and not return from the
if-statement and the problem still exists (the task keep running and
reporting progress even when canceled).
So the issue remains: What to do when the tasks is canceled but doesn't
stop reporting progress and status? Is suppose that we just let it keep
going and leave the decision up to whoever writes the UI. At least they
could gray out the cancel button so the user doesn't keep hammering on
it ...
Steve
On 04/01/2012 2:42 PM, Richard Bair wrote:
> Though once I went down this road, I thought "well, if it is opt-in, then it really isn't buying me much beyond just a runLater which we already have, and if it is opt-out it already breaks people", so I didn't pursue this farther.
>
> On Jan 4, 2012, at 11:40 AM, Richard Bair wrote:
>
>> Right, I wondered about that as well, though in the context of updateProgress(i, 1000, boolean???) or some other way to say "when I invoke this one, I mean it to allow updates after cancel, or not, or whatnot".
>>
>>
>> On Jan 4, 2012, at 11:37 AM, steve.x.northover at oracle.com wrote:
>>
>>> <bogus>
>>> for (iterations=0; iterations<1000; iterations++) }
>>> //ugly API but means that you can't update progress without checking for cancel or setting the message
>>> if (isCancelled("Interation " + i, i) {
>>> break;
>>> }
>>> ... do work ...
>>> }
>>> </bogus>
>>>
>>> Steve
>>>
>>> On 04/01/2012 2:30 PM, Richard Bair wrote:
>>>> What do you mean?
>>>>
>>>> On Jan 4, 2012, at 11:24 AM, steve.x.northover at oracle.com wrote:
>>>>
>>>>> One (wild/bogus/too late?) idea might be to combine checking for cancel with progress update. That way, programmer can't do one without the other.
>>>>>
>>>>> Steve
>>>>>
>>>>> On 04/01/2012 2:11 PM, Richard Bair wrote:
>>>>>> Ya, that was exactly what I was proposing :-). If the updateXXX methods throw an ISE when called from a background thread of a cancelled task, then you will generally get the behavior you instinctively want -- the thread dies (most of the time) and the thing stops getting updated.
>>>>>>
>>>>>> However this means that when you actually DO want to update the progress, message, or title when the thing is cancelled, then you need to do it in the cancelled() method (which is called on the FX app thread) or you have to use a Platform.runLater() if you handle it from the call() method.
>>>>>>
>>>>>> So throwing the exceptions will cause some exceptions in existing apps that weren't happening before (although maybe that is good since it will, if left unhandled, kill the thread). It will make it more difficult for people who are wanting to update the title / message / progress after the thing is cancelled.
>>>>>>
>>>>>> In either case all use cases are possible, it is just a matter of which you bias for. Do you bias for the naive implementation or the sophisticated implementation? Generally I always bias for the naive implementation because even great developers are happier when things "just work" the way their intuition dictates and the exceptions give enough context that developers who want to do the advanced stuff can determine how it is done.
>>>>>>
>>>>>> Richard
>>>>>>
>>>>>>
>>>>>> On Jan 4, 2012, at 11:05 AM, steve.x.northover at oracle.com wrote:
>>>>>>
>>>>>>> Hmmm .. perhaps they should throw an exception, not update the progress and not cancel nicely? I suppose that this "cancels" the task!
>>>>>>>
>>>>>>> Steve
>>>>>>>
>>>>>>> On 04/01/2012 2:00 PM, Richard Bair wrote:
>>>>>>>> Today, they just work (they update the progress, title, message).
>>>>>>>>
>>>>>>>> Richard
>>>>>>>>
>>>>>>>> On Jan 4, 2012, at 10:59 AM, steve.x.northover at oracle.com wrote:
>>>>>>>>
>>>>>>>>> What happens now when updateProgress() and friends are called for a canceled task?
>>>>>>>>>
>>>>>>>>> Steve
>>>>>>>>>
>>>>>>>>> On 04/01/2012 1:55 PM, Richard Bair wrote:
>>>>>>>>>> OK. The only regret I have here is that the fact the faulty code is not the fault of the developer per se, but rather the fault of Java for not allowing the killing of arbitrary threads (and I know we used to allow this and found major problems with it and thus deprecated it, I'm just sayin'). That is, the natural expectation for most people when they first approach it is going to be "hey, I cancelled this thing and it is still running! What the heck??". Then they need to learn more, understand the design space, read the documentation, etc.
>>>>>>>>>>
>>>>>>>>>> My proposed solution makes it work as expected most of the time, and puts the burden on the more sophisticated developers who want to update the progress or message from a background thread. However, since doing nothing but improving documentation means no potential breakage (which introducing an ISE could do), I'm OK with only updating the docs and calling it good :-).
>>>>>>>>>>
>>>>>>>>>> I have added a whole ream of additional samples to Task, and the very first example demonstrates how to do this correctly. Note that the samples team is going to need to update all their code to do this correctly. I haven't tried all these code samples in real life to make sure they all work, I will do so before committing.
>>>>>>>>>>
>>>>>>>>>> *<h2>Examples</h2>
>>>>>>>>>> *<p>
>>>>>>>>>> * The following set of examples demonstrate some of the most common uses of
>>>>>>>>>> * Tasks.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Simple Loop</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * The first example is a simple loop that does nothing particularly useful,
>>>>>>>>>> * but demonstrates the fundamental aspects of writing a Task correctly. This
>>>>>>>>>> * example will simply loop and print to standard out on each loop iteration.
>>>>>>>>>> * When it completes, it returns the number of times it iterated.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * Task<Integer> task = new Task<Integer>() {
>>>>>>>>>> * @@Override protected Integer call() throws Exception {
>>>>>>>>>> * int iterations = 0;
>>>>>>>>>> * for (iterations = 0; iterations< 1000; iterations++) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * System.out.println("Iteration " + iterations);
>>>>>>>>>> * }
>>>>>>>>>> * return iterations;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * First, we define what type of value is returned from this Task. In this
>>>>>>>>>> * case, we want to return the number of times we iterated, so we will
>>>>>>>>>> * specify the Task to be of type Integer by using generics. Then, within
>>>>>>>>>> * the implementation of the<code>call</code> method, we iterate from
>>>>>>>>>> * 0 to 1000. On each iteration, we check to see whether this Task has
>>>>>>>>>> * been cancelled. If it has been, then we break out of the loop and return
>>>>>>>>>> * the number of times we iterated. Otherwise a message is printed to
>>>>>>>>>> * the console and the iteration count increased and we continue looping.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * Checking for isCancelled() in the loop body is critical, otherwise the
>>>>>>>>>> * developer may cancel the task, but the task will continue running
>>>>>>>>>> * and updating both the progress and returning the incorrect result
>>>>>>>>>> * from the end of the<code>call</code> method. A correct implementation
>>>>>>>>>> * of a Task will always check for cancellation.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Simple Loop With Progress Notification</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * Similar to the previous example, except this time we will modify the
>>>>>>>>>> * progress of the Task in each iteration. Note that we have a choice
>>>>>>>>>> * to make in the case of cancellation. Do we want to set the progress back
>>>>>>>>>> * to -1 (indeterminate) when the Task is cancelled, or do we want to leave
>>>>>>>>>> * the progress where it was at? In this case, lets leave the progress alone
>>>>>>>>>> * and only update the message on cancellation, though updating the
>>>>>>>>>> * progress after cancellation is a perfectly valid choice.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * Task<Integer> task = new Task<Integer>() {
>>>>>>>>>> * @@Override protected Integer call() throws Exception {
>>>>>>>>>> * int iterations = 0;
>>>>>>>>>> * for (iterations = 0; iterations< 1000; iterations++) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * updateMessage("Cancelled");
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * updateMessage("Iteration " + iterations);
>>>>>>>>>> * (iterations, 1000);
>>>>>>>>>> * }
>>>>>>>>>> * return iterations;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * As before, within the for loop we check whether the Task has been
>>>>>>>>>> * cancelled. If it has been cancelled, we will update the Task's
>>>>>>>>>> * message to indicate that it has been cancelled, and then break as
>>>>>>>>>> * before. If the Task has not been cancelled, then we will update its
>>>>>>>>>> * message to indicate the current iteration and then update the
>>>>>>>>>> * progress to indicate the current progress.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Simple Loop With Progress Notification And Blocking Calls</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * This example adds to the previous examples a blocking call. Because a
>>>>>>>>>> * blocking call may thrown an InterruptedException, and because an
>>>>>>>>>> * InterruptedException may occur as a result of the Task being cancelled,
>>>>>>>>>> * we need to be sure to handle the InterruptedException and check on the
>>>>>>>>>> * cancel state.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * Task<Integer> task = new Task<Integer>() {
>>>>>>>>>> * @@Override protected Integer call() throws Exception {
>>>>>>>>>> * int iterations = 0;
>>>>>>>>>> * for (iterations = 0; iterations< 1000; iterations++) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * updateMessage("Cancelled");
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * updateMessage("Iteration " + iterations);
>>>>>>>>>> * updateProgress(iterations, 1000);
>>>>>>>>>> *
>>>>>>>>>> * // Now block the thread for a short time, but be sure
>>>>>>>>>> * // to check the interrupted exception for cancellation!
>>>>>>>>>> * try {
>>>>>>>>>> * Thread.sleep(100);
>>>>>>>>>> * } catch (InterruptedException interrupted) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * updateMessage("Cancelled");
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> * return iterations;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * Here we have added to the body of the loop a<code>Thread.sleep</code>
>>>>>>>>>> * call. Since this is a blocking call, I have to handle the potential
>>>>>>>>>> * InterruptedException. Within the catch block, I will check whether
>>>>>>>>>> * the Task has been cancelled, and if so, update the message accordingly
>>>>>>>>>> * and break out of the loop.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Task Which Takes Parameters</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * Most Tasks require some parameters in order to do useful work. For
>>>>>>>>>> * example, a DeleteRecordTask needs the object or primary key to delete
>>>>>>>>>> * from the database. A ReadFileTask needs the URI of the file to be read.
>>>>>>>>>> * Because Tasks operate on a background thread, care must be taken to
>>>>>>>>>> * make sure the body of the<code>call</code> method does not read or
>>>>>>>>>> * modify any shared state. There are two techniques most useful for
>>>>>>>>>> * doing this: using final variables, and passing variables to a Task
>>>>>>>>>> * during construction.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * When using a Task as an anonymous class, the most natural way to pass
>>>>>>>>>> * parameters to the Task is by using final variables. In this example,
>>>>>>>>>> * we pass to the Task the total number of times the Task should iterate.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * final int totalIterations = 900;
>>>>>>>>>> * Task<Integer> task = new Task<Integer>() {
>>>>>>>>>> * @@Override protected Integer call() throws Exception {
>>>>>>>>>> * int iterations = 0;
>>>>>>>>>> * for (iterations = 0; iterations< totalIterations; iterations++) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * updateMessage("Cancelled");
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * updateMessage("Iteration " + iterations);
>>>>>>>>>> * updateProgress(iterations, totalIterations);
>>>>>>>>>> * }
>>>>>>>>>> * return iterations;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * Since<code>totalIterations</code> is final, the<code>call</code>
>>>>>>>>>> * method can safely read it and refer to it from a background thread.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * When writing Task libraries (as opposed to specific-use implementations),
>>>>>>>>>> * we need to use a different technique. In this case, I will create an
>>>>>>>>>> * IteratingTask which performs the same work as above. This time, since
>>>>>>>>>> * the IteratingTask is defined in its own file, it will need to have
>>>>>>>>>> * parameters passed to it in its constructor. These parameters are
>>>>>>>>>> * assigned to final variables.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * public class IteratingTask extends Task<Integer> {
>>>>>>>>>> * private final int totalIterations;
>>>>>>>>>> *
>>>>>>>>>> * public IteratingTask(int totalIterations) {
>>>>>>>>>> * this.totalIterations = totalIterations;
>>>>>>>>>> * }
>>>>>>>>>> *
>>>>>>>>>> * @@Override protected Integer call() throws Exception {
>>>>>>>>>> * int iterations = 0;
>>>>>>>>>> * for (iterations = 0; iterations< totalIterations; iterations++) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * updateMessage("Cancelled");
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * updateMessage("Iteration " + iterations);
>>>>>>>>>> * updateProgress(iterations, totalIterations);
>>>>>>>>>> * }
>>>>>>>>>> * return iterations;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>And then when used:</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * IteratingTask task = new IteratingTask(800);
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>In this way, parameters are passed to the IteratingTask in a safe
>>>>>>>>>> * manner, and again, are final. Thus, the<code>call</code> method can
>>>>>>>>>> * safely read this state from a background thread.</p>
>>>>>>>>>> *
>>>>>>>>>> *<p>WARNING: Do not pass mutable state to a Task and then operate on it
>>>>>>>>>> * from a background thread. Doing so may introduce race conditions. In
>>>>>>>>>> * particular, suppose you had a SaveCustomerTask which took a Customer
>>>>>>>>>> * in its constructor. Although the SaveCustomerTask may have a final
>>>>>>>>>> * reference to the Customer, if the Customer object is mutable, then it
>>>>>>>>>> * is possible that both the SaveCustomerTask and some other application code
>>>>>>>>>> * will be reading or modifying the state of the Customer from different
>>>>>>>>>> * threads. Be very careful in such cases, that while a mutable object such
>>>>>>>>>> * as this Customer is being used from a background thread, that it is
>>>>>>>>>> * not being used also from another thread. In particular, if the background
>>>>>>>>>> * thread is reading data from the database and updating the Customer object,
>>>>>>>>>> * and the Customer object is bound to scene graph nodes (such as UI
>>>>>>>>>> * controls), then there could be a violation of threading rules! For such
>>>>>>>>>> * cases, modify the Customer object from the FX Application Thread rather
>>>>>>>>>> * than from the background thread.</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * public class UpdateCustomerTask extends Task<Customer> {
>>>>>>>>>> * private final Customer customer;
>>>>>>>>>> *
>>>>>>>>>> * public UpdateCustomerTask(Customer customer) {
>>>>>>>>>> * this.customer = customer;
>>>>>>>>>> * }
>>>>>>>>>> *
>>>>>>>>>> * @@Override protected Customer call() throws Exception {
>>>>>>>>>> * // pseudo-code:
>>>>>>>>>> * // query the database
>>>>>>>>>> * // read the values
>>>>>>>>>> *
>>>>>>>>>> * // Now update the customer
>>>>>>>>>> * Platform.runLater(new Runnable() {
>>>>>>>>>> * @@Override public void run() {
>>>>>>>>>> * customer.firstName(rs.getString("FirstName"));
>>>>>>>>>> * // etc
>>>>>>>>>> * }
>>>>>>>>>> * });
>>>>>>>>>> *
>>>>>>>>>> * return customer;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Task Which Returns No Value</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * Many, if not most, Tasks should return a value upon completion. For
>>>>>>>>>> * CRUD Tasks, one would expect that a "Create" Task would return the newly
>>>>>>>>>> * created object or primary key, a "Read" Task would return the read
>>>>>>>>>> * object, an "Update" task would return the number of records updated,
>>>>>>>>>> * and a "Delete" task would return the number of records deleted.
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<p>
>>>>>>>>>> * However sometimes there just isn't anything truly useful to return.
>>>>>>>>>> * For example, I might have a Task which writes to a file. Task has built
>>>>>>>>>> * into it a mechanism for indicating whether it has succeeded or failed
>>>>>>>>>> * along with the number of bytes written (the progress), and thus there is
>>>>>>>>>> * nothing really for me to return. In such a case, you can use the Void
>>>>>>>>>> * type. This is a special type in the Java language which can only be
>>>>>>>>>> * assigned the value of<code>null</code>. You would use it as follows:
>>>>>>>>>> *</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * final String filePath = "/foo.txt";
>>>>>>>>>> * final String contents = "Some contents";
>>>>>>>>>> * Task<Void> task = new Task<Void>() {
>>>>>>>>>> * @@Override protected Void call() throws Exception {
>>>>>>>>>> * File file = new File(filePath);
>>>>>>>>>> * FileOutputStream out = new FileOutputStream(file);
>>>>>>>>>> * // ... and other code to write the contents ...
>>>>>>>>>> *
>>>>>>>>>> * // Return null at the end of a Task of type Void
>>>>>>>>>> * return null;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Task Which Returns An ObservableList</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>Because the ListView, TableView, and other UI controls and scene graph
>>>>>>>>>> * nodes make use of ObservableList, it is common to want to create and return
>>>>>>>>>> * an ObservableList from a Task. When you do not care to display intermediate
>>>>>>>>>> * values, the easiest way to correctly write such a Task is simply to
>>>>>>>>>> * construct an ObservableList within the<code>call</code> method, and then
>>>>>>>>>> * return it at the conclusion of the Task.</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * Task<ObservableList<Rectangle>> task = new Task<ObservableList<Rectangle>>() {
>>>>>>>>>> * @@Override protected ObservableList<Rectangle> call() throws Exception {
>>>>>>>>>> * ObservableList<Rectangle> results = FXCollections.observableArrayList();
>>>>>>>>>> * for (int i=0; i<100; i++) {
>>>>>>>>>> * if (isCancelled()) break;
>>>>>>>>>> * Rectangle r = new Rectangle(10, 10);
>>>>>>>>>> * r.setX(10 * i);
>>>>>>>>>> * results.add(r);
>>>>>>>>>> * }
>>>>>>>>>> * return results;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<p>In the above example, we are going to create 100 rectangles and return
>>>>>>>>>> * them from this task. An ObservableList is created within the
>>>>>>>>>> *<code>call</code> method, populated, and then returned.</p>
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Task Which Returns Partial Results</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>Sometimes you want to create a Task which will return partial results.
>>>>>>>>>> * Perhaps you are building a complex scene graph and want to show the
>>>>>>>>>> * scene graph as it is being constructed. Or perhaps you are reading a large
>>>>>>>>>> * amount of data over the network and want to display the entries in a
>>>>>>>>>> * TableView as the data is arriving. In such cases, there is some shared state
>>>>>>>>>> * available both to the FX Application Thread and the background thread.
>>>>>>>>>> * Great care must be taken to<strong>never update shared state from any
>>>>>>>>>> * thread other than the FX Application Thread</strong>.</p>
>>>>>>>>>> *
>>>>>>>>>> *<p>The easiest way to do this is to expose a new property on the Task
>>>>>>>>>> * which will represent the partial result. Then make sure to use
>>>>>>>>>> *<code>Platform.runLater</code> when adding new items to the partial
>>>>>>>>>> * result.</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * public class PartialResultsTask extends Task<ObservableList<Rectangle>> {
>>>>>>>>>> * // Uses Java 7 diamond operator
>>>>>>>>>> * private ReadOnlyObjectWrapper<ObservableList<Rectangle>> partialResults =
>>>>>>>>>> * new ReadOnlyObjectWrapper<>(this, "partialResults");
>>>>>>>>>> *
>>>>>>>>>> * public final ObservableList<Rectangle> getPartialResults() { return partialResults; }
>>>>>>>>>> * public final ReadOnlyObjectProperty<ObservableList<Rectangle>> partialResultsProperty() {
>>>>>>>>>> * return partialResults.getReadOnlyProperty();
>>>>>>>>>> * }
>>>>>>>>>> *
>>>>>>>>>> * @@Override protected ObservableList<Rectangle> call() throws Exception {
>>>>>>>>>> * for (int i=0; i<100; i++) {
>>>>>>>>>> * if (isCancelled()) break;
>>>>>>>>>> * final Rectangle r = new Rectangle(10, 10);
>>>>>>>>>> * r.setX(10 * i);
>>>>>>>>>> * Platform.runLater(new Runnable() {
>>>>>>>>>> * @@Override public void run() {
>>>>>>>>>> * partialResults.add(r);
>>>>>>>>>> * }
>>>>>>>>>> * });
>>>>>>>>>> * }
>>>>>>>>>> * return partialResults;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<!-- TODO: Update to use the PartialResultsTask. This needs to be a new
>>>>>>>>>> * task written such that there is an updateResults method which does
>>>>>>>>>> * all the normal event queue coalescing and so forth. Creating a
>>>>>>>>>> * PartialResultsTask is trivial, however creating a PartialResultsService
>>>>>>>>>> * is a bit more intimidating. The problem with PartialResultsService is
>>>>>>>>>> * that you might also want a PartialResultsScheduledService, so there is
>>>>>>>>>> * some explosion of API possible. It might be that we simply teach
>>>>>>>>>> * Service itself how to deal with PartialResultsTasks -- that is probably
>>>>>>>>>> * my preferred approach. -->
>>>>>>>>>> *
>>>>>>>>>> *<h3>A Task Which Modifies The Scene Graph</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>Generally, Tasks should not interact directly with the UI. Doing so
>>>>>>>>>> * creates a tight coupling between a specific Task implementation and a
>>>>>>>>>> * specific part of your UI. However, when you do want to create such a
>>>>>>>>>> * coupling, you must ensure that you use<code>Platform.runLater</code>
>>>>>>>>>> * so that any modifications of the scene graph occur on the
>>>>>>>>>> * FX Application Thread.</p>
>>>>>>>>>> *
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * final Group group = new Group();
>>>>>>>>>> * Task<Void> task = new Task<Void>() {
>>>>>>>>>> * @@Override protected Void call() throws Exception {
>>>>>>>>>> * for (int i=0; i<100; i++) {
>>>>>>>>>> * if (isCancelled()) break;
>>>>>>>>>> * final Rectangle r = new Rectangle(10, 10);
>>>>>>>>>> * r.setX(10 * i);
>>>>>>>>>> * Platform.runLater(new Runnable() {
>>>>>>>>>> * @@Override public void run() {
>>>>>>>>>> * group.getChildren().add(r);
>>>>>>>>>> * }
>>>>>>>>>> * });
>>>>>>>>>> * }
>>>>>>>>>> * return null;
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>> *
>>>>>>>>>> *<h3>Reacting To State Changes Generically</h3>
>>>>>>>>>> *
>>>>>>>>>> *<p>Sometimes you may want to write a Task which updates its progress,
>>>>>>>>>> * message, text, or in some other way reacts whenever a state change
>>>>>>>>>> * happens on the Task. For example, you may want to change the status
>>>>>>>>>> * message on the Task on Failure, Success, Running, or Cancelled state changes.
>>>>>>>>>> *</p>
>>>>>>>>>> *<pre><code>
>>>>>>>>>> * Task<Integer> task = new Task<Integer>() {
>>>>>>>>>> * @@Override protected Integer call() throws Exception {
>>>>>>>>>> * int iterations = 0;
>>>>>>>>>> * for (iterations = 0; iterations< 1000; iterations++) {
>>>>>>>>>> * if (isCancelled()) {
>>>>>>>>>> * break;
>>>>>>>>>> * }
>>>>>>>>>> * System.out.println("Iteration " + iterations);
>>>>>>>>>> * }
>>>>>>>>>> * return iterations;
>>>>>>>>>> * }
>>>>>>>>>> *
>>>>>>>>>> * @@Override protected void succeeded() {
>>>>>>>>>> * super.succeeded();
>>>>>>>>>> * updateMessage("Done!");
>>>>>>>>>> * }
>>>>>>>>>> *
>>>>>>>>>> * @@Override protected void cancelled() {
>>>>>>>>>> * super.cancelled();
>>>>>>>>>> * updateMessage("Cancelled!");
>>>>>>>>>> * }
>>>>>>>>>> *
>>>>>>>>>> * @@Override protected void failed() {
>>>>>>>>>> * super.failed();
>>>>>>>>>> * updateMessage("Failed!");
>>>>>>>>>> * }
>>>>>>>>>> * }
>>>>>>>>>> *</code></pre>
>>>>>>>>>>
>>>>>>>>>> You'll notice that I also included a comment there about a PartialResultsTask. While writing the above it occurred to me that the partial results use case is not very elegant at the moment. I have some thoughts on that I was playing with which I'll post for review when I've thought about it more.
>>>>>>>>>>
>>>>>>>>>> So the current proposal for this issue is: close as not an issue, and improve the documentation as above.
>>>>>>>>>>
>>>>>>>>>> Thanks everybody for your very valuable input!
>>>>>>>>>> Richard
>>>>>>>>>>
>>>>>>>>>> On Jan 4, 2012, at 7:49 AM, steve.x.northover at oracle.com wrote:
>>>>>>>>>>
>>>>>>>>>>> +1 for not fixing faulty code. People who write background task code need to play by the rules and check for their task being canceled.
>>>>>>>>>>>
>>>>>>>>>>> Steve
>>>>>>>>>>>
>>>>>>>>>>> On 04/01/2012 8:11 AM, Roman Kennke wrote:
>>>>>>>>>>>> Hi Richard,
>>>>>>>>>>>>
>>>>>>>>>>>> I agree with the other's sentiments that JavaFX should not try to fix
>>>>>>>>>>>> faulty code.
>>>>>>>>>>>>
>>>>>>>>>>>> I would lean to do more or less what is done in java.util.concurrent,
>>>>>>>>>>>> especially the FutureTask class. I.e. provide an API to check
>>>>>>>>>>>> isCancelled() and otherwise use InterruptionException to correctly
>>>>>>>>>>>> interrupt a thread:
>>>>>>>>>>>>
>>>>>>>>>>>> http://www.ibm.com/developerworks/java/library/j-jtp05236/index.html
>>>>>>>>>>>>
>>>>>>>>>>>> In my experience, APIs that let people write shitty code leads to a lot
>>>>>>>>>>>> of shitty code being written (surprise!), especially when threads are
>>>>>>>>>>>> involved.
>>>>>>>>>>>>
>>>>>>>>>>>> Cheers, Roman
>>>>>>>>>>>>
>>>>>>>>>>>> Am Dienstag, den 03.01.2012, 20:05 -0800 schrieb Richard Bair:
>>>>>>>>>>>>> http://javafx-jira.kenai.com/browse/RT-17932
>>>>>>>>>>>>>
>>>>>>>>>>>>> I have a proposed solution for this bug, if anybody has a few minutes to give it some thought that would be great. In a nutshell:
>>>>>>>>>>>>>
>>>>>>>>>>>>> - When a task is cancelled, the background thread might continue to operate. This is the way Java works (we can't safely kill a thread outright, so we have to wait for the thread to terminate)
>>>>>>>>>>>>> - The developer of a Task can check isCancelled from the background thread and self-terminate
>>>>>>>>>>>>> - The developer can use any API that throws an InterruptedException and use that to indicate that the thread has been cancelled
>>>>>>>>>>>>> - A "run-away" background thread that is supposed to be cancelled, can continue to update the text, message, and progress of a task, giving the appearance that it isn't cancelled!
>>>>>>>>>>>>> - A cancelled task might want to legitimately update the text, message, or progress in the case of cancellation (maybe the message becomes "I'm cancelling this request").
>>>>>>>>>>>>>
>>>>>>>>>>>>> In trying to solve this problem, I wanted a solution where the naive implementation still works as one would expect, and a well considered solution works perfectly. I think I have a solution.
>>>>>>>>>>>>>
>>>>>>>>>>>>> If you write a Task, and do nothing smart:
>>>>>>>>>>>>>
>>>>>>>>>>>>> new Task() {
>>>>>>>>>>>>> public Object call() throws Exception {
>>>>>>>>>>>>> for (int i=0; i<100; i++) {
>>>>>>>>>>>>> doSomething();
>>>>>>>>>>>>> }
>>>>>>>>>>>>> return null;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> In this case, if the task is cancelled, then it will just keep running in the background, but since the task returns nothing and never updates the progress or text or message, it appears to have been successfully cancelled to any code using the task. Here is another dumb implementation:
>>>>>>>>>>>>>
>>>>>>>>>>>>> new Task() {
>>>>>>>>>>>>> public Object call() throws Exception {
>>>>>>>>>>>>> for (int i=0; i<100; i++) {
>>>>>>>>>>>>> doSomething();
>>>>>>>>>>>>> updateProgress(i, 100);
>>>>>>>>>>>>> }
>>>>>>>>>>>>> return null;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> This one will update the progress, and exhibits the bug. My proposal is that calling updateProgress, updateMessage, or updateText from a background thread of a cancelled Task will result in an IllegalStateException. So in this case, the attempt to updateProgress after the task was cancelled will actually cause the exception and, in this case, actually terminate the background thread. Two birds, one stone.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In this case...:
>>>>>>>>>>>>>
>>>>>>>>>>>>> new Task() {
>>>>>>>>>>>>> public Object call() throws Exception {
>>>>>>>>>>>>> for (int i=0; i<100; i++) {
>>>>>>>>>>>>> try {
>>>>>>>>>>>>> doSomething();
>>>>>>>>>>>>> updateProgress(i, 100);
>>>>>>>>>>>>> } catch (Exception e) { }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> return null;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> ...we are not so lucky. Although the progress won't be updated on a cancelled task, it will keep running in the background. But no harm, no foul. If the developer was smart they'd do something like:
>>>>>>>>>>>>>
>>>>>>>>>>>>> new Task() {
>>>>>>>>>>>>> public Object call() throws Exception {
>>>>>>>>>>>>> for (int i=0; i<100; i++) {
>>>>>>>>>>>>> if (isCancelled()) break;
>>>>>>>>>>>>> try {
>>>>>>>>>>>>> doSomething();
>>>>>>>>>>>>> updateProgress(i, 100);
>>>>>>>>>>>>> } catch (Exception e) { }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> return null;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> Anyway, it seems that this solution doesn't hurt the naive case, and in some instances helps it, and in any case for more sophisticated library developers you have the tools to handle cancellation correctly. Here are two ways to correctly update the text after the task has been cancelled:
>>>>>>>>>>>>>
>>>>>>>>>>>>> new Task() {
>>>>>>>>>>>>> public Object call() throws Exception {
>>>>>>>>>>>>> for (int i=0; i<100; i++) {
>>>>>>>>>>>>> if (isCancelled()) {
>>>>>>>>>>>>> Platform.runLater(new Runnable() {
>>>>>>>>>>>>> public void run() {
>>>>>>>>>>>>> updateMessage("Cancelled")
>>>>>>>>>>>>> }
>>>>>>>>>>>>> });
>>>>>>>>>>>>> break;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> try {
>>>>>>>>>>>>> doSomething();
>>>>>>>>>>>>> updateProgress(i, 100);
>>>>>>>>>>>>> } catch (Exception e) { }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> return null;
>>>>>>>>>>>>> }
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> new Task() {
>>>>>>>>>>>>> public Object call() throws Exception {
>>>>>>>>>>>>> for (int i=0; i<100; i++) {
>>>>>>>>>>>>> if (isCancelled()) break;
>>>>>>>>>>>>> try {
>>>>>>>>>>>>> doSomething();
>>>>>>>>>>>>> updateProgress(i, 100);
>>>>>>>>>>>>> } catch (Exception e) { }
>>>>>>>>>>>>> }
>>>>>>>>>>>>> return null;
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> @Override protected void cancelled() {
>>>>>>>>>>>>> updateMessage("Cancelled");
>>>>>>>>>>>>> }
>>>>>>>>>>>>> };
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think this works fairly well. Anything I'm missing?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>> Richard
More information about the openjfx-dev
mailing list