Config self reference fix

[ahgittin]

builds on #462 fixing the remaining issues discussed in BROOKLYN-329

immediate eval could be improved yet further but this fixes scope/resolution issues and in general makes immediate things much more reliable, as well as fixing recursive references in config settings, and helping to debug bad task traces

[aledsage]

@ahgittin This is way too hard to review until #462 is merged (which requires rebasing and a bunch of PR comments to be addressed; as I asked there, will you have time to address those or would you like one of us to pick it up)?

This PR also looks like it has a bunch of other commits that could (and should) be extracted into their own PRs (e.g. "solve problem with map eval" perhaps? Definitely "slow tests moved to integration group"? "cleanup, and allow TaskFactory to be supplied as a config..." perhaps?). I presume they are not all related to the title "Config self reference fix"?

Including many changes in a single PR makes the reviewers job much harder, and makes it take much much longer to get things merged.

[ahgittin]

Rebased, and once #462 is merged this should be easier to review. Cherry-picking my "slow tests" meant it automatically left this PR which was nice.

[ahgittin]

Re TaskFactory being a separate PR, that would mean massaging the tests and other places so there is a clean state where TF isn't supported, and a clean state where TF is supported. Agree in general PRs should be split up, but where something is trivial (integration tests), or where things are entangled (TaskFactory) it should be a judgment call. We should avoid hypothetical problem-spotting: "Your PR includes things not 100% related. That's scientifically proven to make it much harder to review." If in the course of a review it is actually hard, then ask for a tidy-up. I might get the balance wrong sometimes but I think here my atrocities are limited to having neglected #462 for months and months.

[ahgittin]

Pretty sure the failures reported here are unrelated:

* org.apache.brooklyn.launcher.CleanOrphanedLocationsIntegrationTest.testCleanedCopiedPersistedState
* org.apache.brooklyn.launcher.CleanOrphanedLocationsIntegrationTest.testSelectionWithOrphanedLocationsInData

Both pass for me locally. (Incidentally @aledsage none of the tests in that class are marked "Integration".)

[ahgittin]

Unrelated test failures in penultimate jenkins build are discussed at #560

[aledsage]

Thanks @ahgittin - looks great.

Most of my comments are minor - happy for you to use your judgement for what is worth changing.

I'd appreciate a description of how EntityConfigTest.testGetTaskNonBlockingMap() now passes - what prevents the underlying task from being cancelled?

[aledsage]

Worth adding javadoc here - e.g. similar to what you've added in the impl BasicExecutionContext. Worth saying when it will return Maybe.absent (e.g. if the task execution requires blocking for other work, and can't complete in a timely fashion).

[ahgittin]

good idea. we should maybe move ImmediateSupplier to the utils package, then we could reference its javadoc? we might also change the return type to be ReferenceWithError<Maybe<T>> so the "can't immediately tell if there's a value" problem state can be detected without throwing. have marked @Beta for now.

[aledsage]

If I'm reading this right... it adds the tag to the current task, but then the tag is not removed at the end of this method - should it be?

[ahgittin]

the tag is left on the task. you're right that could cause problems if the calling code isn't in a task (maybe it always will be but safer not to assume).

UPDATE: we now always use a dedicated tag

[ahgittin]

actually i've done a better strategy -- @aledsage appreciate any thoughts on this:

                // don't check on ourself; only look higher in hierarchy;
                // this assumes impls always spawn new tasks (which they do, just maybe not always in new threads)
                // but it means it does not rely on tag removal to prevent weird errors, 
                // and more importantly it makes the strategy idempotent

[ahgittin]

but actually this won't work either will it --

consider P1 needs to evaluate key C1, then key C2, and C2 refers to C1. if it isn't a dedicated subtask the second check will fail.

guess we need to ensure a dedicated subtask. :( . i'll see whether we can do that, probably with an "official" task tag type.

[ahgittin]

did it, but without an official task type -- easy enough to get the routines to share code and trigger a dedicated (non-thread) task

[aledsage]

This test passes for me - when/why does it fail, or does this comment need deleted?

[ahgittin]

stale comment, good catch

[aledsage]

Delete commented out code, or add additional comment to say when one would uncomment it.

[aledsage]

Are you calling this to clear the interrupted status? Why? Do you get an ugly exception or something if we don't?

[ahgittin]

exactly, comment added

[aledsage]

On balance I'm fine with this because we use the same pattern in lots of places in Brooklyn. But I think it's a bad pattern - it hides programming errors where null shouldn't have been passed in, in exchange for a small number of callers getting to save a few characters by skipping their null check. The former are so much more effort to fix that it's worth putting up with the latter.

[aledsage]

I'd add something like the following to ValueResolverTest:

    public void testTaskFactoryGet() {
        TaskFactory<TaskAdaptable<String>> taskFactory = new TaskFactory<TaskAdaptable<String>>() {
            @Override public TaskAdaptable<String> newTask() {
                return new BasicTask<>(Callables.returning("myval"));
            }
        };
        String result = Tasks.resolving(taskFactory).as(String.class).context(app).get();
        assertEquals(result, "myval");
    }
    
    public void testTaskFactoryGetImmediately() {
        TaskFactory<TaskAdaptable<String>> taskFactory = new TaskFactory<TaskAdaptable<String>>() {
            @Override public TaskAdaptable<String> newTask() {
                return new BasicTask<>(Callables.returning("myval"));
            }
        };
        String result = Tasks.resolving(taskFactory).as(String.class).context(app).immediately(true).get();
        assertEquals(result, "myval");
    }
    
    public void testTaskFactoryGetImmediatelyDoesNotBlock() {
        final AtomicBoolean executing = new AtomicBoolean();
        TaskFactory<TaskAdaptable<String>> taskFactory = new TaskFactory<TaskAdaptable<String>>() {
            @Override public TaskAdaptable<String> newTask() {
                return new BasicTask<>(new Callable<String>() {
                    public String call() {
                        executing.set(true);
                        try {
                            Time.sleep(Duration.ONE_MINUTE);
                            return "myval";
                        } finally {
                            executing.set(false);
                        }
                    }});
            }
        };
        Maybe<String> result = Tasks.resolving(taskFactory).as(String.class).context(app).immediately(true).getMaybe();
        assertTrue(result.isAbsent(), "result="+result);
        Asserts.succeedsEventually(new Runnable() {
            public void run() {
                assertFalse(executing.get());
            }
        });
    }

However, the last assertion fails - using immediately() with TaskFactory is dangerous because the task is left running. It is particularly dangerous because immediately() is most often used when it's being called regularly (e.g. the rest api wants to list all the config values).

Whose responsibility is it to cancel the task? (It feels like the task instance is buried inside the ValueResolver, so it would be hard for the caller to get hold of it to cancel it). Should the ValueResolver cancel the task it created, if using immediately()?

What are the use-cases for passing a TaskFactory into the ValueResolver?

[ahgittin]

TODO think about this

[aledsage]

I suspect our DependentConfiguration class could do with a re-write, but not sure off-hand exactly what it should look like. And doesn't feel like it's worth doing right now.

It's trick because WaitInTaskForAttributeReady extends Callable expects to subscribe to (multiple) sensors, and then block for a matching value. We could most likely re-implement that (see below).

Using a TaskFactory would be tricky, because we only want to do the subscriptions once. It is conceptually a single "task" (i.e. we don't want to do the subscriptions multiple times).

My gut-feel is therefore we could implement it as a DeferredSupplier. When first called (or maybe when created), it would create the subscriptions. The thread executing the get would just be blocking on a CountDownLatch, which would be set by the subscription callbacks when the value was available (or when the entity had failed). Multiple calls to get() would share the same instance, so would all be blocking on the same thing.

[ahgittin]

Interesting idea that it is a single (shared) task. I feel like each caller gets its own instance/subscription hence TaskFactory. But for another day... (and in any case I'd love us to move to a JS promise/then model)

[aledsage]

Why does this now work? We still have the call to task.cancel(true) in AbstractConfigurationSupportInternal.getNonBlockingResolvingStructuredKey(), so during the first call to getNonBlocking we'll cancel the task. Is it because that is a different task that does not pass the cancel() call down to the actual config-key-value task that we are trying to evaluate?

[ahgittin]

alex TODO investigate this, and testGetTaskNonBlockingMap(), and also there is still one map test failing, investigate it also

[aledsage]

Add to ExceptionsTest:

    @Test
    public void testGetCausalChain() throws Exception {
        Exception e1 = new Exception("e1");
        Exception e2 = new Exception("e2", e1);
        assertEquals(Exceptions.getCausalChain(e2), ImmutableList.of(e2, e1));
    }
    
    @Test
    public void testGetCausalChainRecursive() throws Exception {
        Exception e1 = new Exception("e1") {
            public synchronized Throwable getCause() {
                return this;
            }
        };
        Exception e2 = new Exception("e2", e1);
        assertEquals(Exceptions.getCausalChain(e2), ImmutableList.of(e2, e1));
    }

[ahgittin]

thanks @aledsage -- have done most of them. a few more will require more thought from me, marked them so i can find them easily.

[ahgittin]

All should be fixed now @aledsage

99ccc0f adds comments and assertions for the cancellation behaviour. TL;DR, TaskFactory config values should have their tasks cancelled, but Task values should not. The last commit fixes one thing which had been causing a failure in integration tests.

The remaining messy/broken tests seem to be mainly around immediate not having any guarantees due to timing. I'd like to try to switch most/all to use the interrupting-immediate pattern, but that'll be another PR :) .

[aledsage]

Looking good - I've written a few more tests that fail (pasted into the comments). This is complicated stuff!

How do you want to proceed, @ahgittin? Do we need those new tests added and fixed before we merge this? Or should we merge this (with the new tests added and marked @Broken) and come back to it in a separate PR?

[aledsage]

Minor: I'd have included a comment on this method to say that if immediate then the return value will be Maybe<Object> whereas if !immediate then the return value of the callable will be the actual Object. The difference of those semantics are not obvious in the method, but are relied upon by the caller.

It's surprising that the same method is used to return the two different styles of result, but I see why you did it (to reduce duplication).

[ahgittin]

yeah it's ugly; at least it's private. have renamed.

[aledsage]

Under what code path do we get here? Looking at ValueResolver.getMaybeInternal(...), allowImmediateExecution will be false if the object is a task (rather than a TaskFactory or DeferredSupplier).

For example, the test below fails (added to ValueResolverTest). It leaves a single instance of the task executing.

    public void testTaskGetImmediatelyDoesNotBlock() {
        final AtomicInteger executingCount = new AtomicInteger();
        
        final Task<String> task = new BasicTask<>(new Callable<String>() {
            public String call() {
                executingCount.incrementAndGet();
                try {
                    Time.sleep(Duration.ONE_MINUTE);
                    return "myval";
                } finally {
                    executingCount.decrementAndGet();
                }
            }});
        
        for (int i = 0; i < 3; i++) {
            Maybe<String> result = Tasks.resolving(task).as(String.class).context(app).immediately(true).getMaybe();
            Asserts.assertTrue(result.isAbsent(), "result="+result);
        }

        Asserts.assertFalse(task.isSubmitted());
        
        // The call below default times out after 30s while the task above is still running
        // Expect the task to not be left running.
        Asserts.succeedsEventually(new Runnable() {
            public void run() {
                Asserts.assertEquals(executingCount.get(), 0);
            }
        });
    }

I'm not convinced that we want to support setting a config key with a value of type Task (long term). But I guess that will require more work, to change how DependentConfiguration is implemented. So we do need to support it short-term :-(

[aledsage]

If I change ValueResolver.getMaybeInternal() to set allowImmediateExecution = true; when it is passed a TaskAdaptable, then the above test does pass.

[ahgittin]

as noted above leaving the task running in background (or possibly not submitting it) is the right thing to do for immediate execution of a task; cancelling it is risky unless we know the task is dedicated to this one usage (hence preferring TaskFactory); in particular EntityConfigTest.testGetTaskNonBlockingKey() fails if allowImmediateExecution=true

[ahgittin]

As for the particular code block marked here, it is invoked from ValueResolver when you supply a TaskFactory; VR generates the Task and passes it in with allowImmediateExecution=true

[aledsage]

Very clever, getting the underlying Callable to execute from the job!

However, I wonder if this just reduces the chance of leaving tasks behind (rather than preventing it).

The test below fails when added to ValueResolverTest (when also including the change to ValueResolver.getMaybeInternal() to set allowImmediateExecution = true so that this code path is taken).

    public void testTaskGetImmediatelyDoesNotBlockWithNestedTasks() {
        final AtomicInteger executingCount = new AtomicInteger();
        
        final SequentialTask<?> outerTask = new SequentialTask<>(ImmutableList.of(new Callable<String>() {
            public String call() {
                executingCount.incrementAndGet();
                try {
                    Time.sleep(Duration.ONE_MINUTE);
                    return "myval";
                } finally {
                    executingCount.decrementAndGet();
                }
            }}));
        
        for (int i = 0; i < 3; i++) {
            Maybe<String> result = Tasks.resolving(outerTask).as(String.class).context(app).immediately(true).getMaybe();
            Asserts.assertTrue(result.isAbsent(), "result="+result);
        }
        
        Asserts.assertFalse(outerTask.isSubmitted());
        
        // the call below default times out after 30s while the task above is still running
        Asserts.succeedsEventually(new Runnable() {
            public void run() {
                Asserts.assertEquals(executingCount.get(), 0);
            }
        });
    }

[ahgittin]

as above

[aledsage]

When playing around with the extra tests (see previous comments), do we need to add allowImmediateExecution = true; when v instanceof TaskAdaptable? Or will that have other unwanted? side-effects?

[ahgittin]

as above, definitely not

[aledsage]

Another failing test I wrote in ValueResolverTest, which leaks tasks (because the outer task is cancelled, while the inner tasks are not).

I'm a bit surprised that it fails - I had assumed that the SequentialTask would cancel its children when it was cancelled.

    public void testTaskFactoryGetImmediatelyDoesNotBlockWithNestedTasks() {
        final int NUM_CALLS = 3;
        final AtomicInteger executingCount = new AtomicInteger();
        final List<SequentialTask<?>> outerTasks = Lists.newArrayList();
        
        TaskFactory<Task<?>> taskFactory = new TaskFactory<Task<?>>() {
            @Override public Task<?> newTask() {
                SequentialTask<?> result = new SequentialTask<>(ImmutableList.of(new Callable<String>() {
                    public String call() {
                        executingCount.incrementAndGet();
                        try {
                            Time.sleep(Duration.ONE_MINUTE);
                            return "myval";
                        } finally {
                            executingCount.decrementAndGet();
                        }
                    }}));
                outerTasks.add(result);
                return result;
            }
        };
        for (int i = 0; i < NUM_CALLS; i++) {
            Maybe<String> result = Tasks.resolving(taskFactory).as(String.class).context(app).immediately(true).getMaybe();
            Asserts.assertTrue(result.isAbsent(), "result="+result);
        }
        // the call below default times out after 30s while the task above is still running
        Asserts.succeedsEventually(new Runnable() {
            public void run() {
                Asserts.assertEquals(outerTasks.size(), NUM_CALLS);
                for (Task<?> task : outerTasks) {
                    Asserts.assertTrue(task.isDone());
                    Asserts.assertTrue(task.isCancelled());
                }
            }
        });
        Asserts.succeedsEventually(new Runnable() {
            public void run() {
                Asserts.assertEquals(executingCount.get(), 0);
            }
        });
    }

[ahgittin]

DST should cancel -- this looks like a bug

[ahgittin]

aha, DST which we mostly use is fine, but its doCancel method was missing from the lighter-weight CompoundTask; have added it there and test passes

[aledsage]

typo: be thrown.
And could add javadoc:

    /**
     * @throws InterruptingImmediateSupplier.InterruptingImmediateSupplierNotSupportedForObject if ...
     */

[aledsage]

I'd prefer this to propagate the exception, rather than return null. I don't like the use of null to indicate that it couldn't be evaluated.

[ahgittin]

+1

[ahgittin]

@aledsage Thanks for thorough comments. I'll review this afternoon and revert with whether to merge or not.

One high level comment: Task instances should be left running, even if immediate execution is requested, because someone else might subsequently ask for that value in a non-immediate context. If we cancelled after immediate execution then the subsequent request would fail. This isn't a leak but an implication of supporting Task evaluation (and for that reason I think we want to prefer TaskFactory evaluation). I think this addresses many of your comments; will look closer after lunch.

[ahgittin]

@aledsage I think I've addressed these comments, TY; but now merge conflicts with #155 are hurting; as noted there I think we should revert that.

[ahgittin]

@aledsage minor changes addressing your comments, and fixed conflicts; would like your confirmation that an immediate Task "leaking" is the right behaviour (and longer term moving away from tasks as these values!) and anything else on your mind, but feels like it may be good to merge now :)

[aledsage]

@ahgittin agreed - we should not cancel the task, so "leaking" it is right behaviour.

The build failure is real, until we commit to switching to Java 8 - please give your opinion on that email thread :-)

[ERROR] /home/jenkins/jenkins-slave/workspace/brooklyn-server-pull-requests/core/src/main/java/org/apache/brooklyn/util/core/task/BasicExecutionContext.java:[31,25] error: package java.util.function does not exist

That package was added in Java 8.

I think it's just the wrong import (should be guava's; it's only used for the javadoc).

Can you fix that, and then merge?

[ahgittin]

thanks @aledsage - fixed, if these tests pass, then good to merge

[aledsage]

Excellent, thanks @ahgittin - merging now.