Skip to content
Permalink
Browse files

[JENKINS-42511] Change the ComputedFolder API to define locking behav…

…iour between events and computations
  • Loading branch information
stephenc committed Mar 7, 2017
1 parent 9f90a8f commit 0d8d28063afbcfc94b7aa455e97e1c31dce0ea1a
@@ -27,56 +27,84 @@
import hudson.model.Item;
import hudson.model.TaskListener;
import hudson.model.TopLevelItem;
import java.io.IOException;
import java.util.Map;
import java.util.Set;
import javax.annotation.CheckForNull;

/**
* Callback for {@link ComputedFolder}.
* Methods may be called only inside the scope of {@link ComputedFolder#computeChildren} or an out-of-band event handler.
* Callback for {@link ComputedFolder}. Methods may be called only inside the scope of
* {@link ComputedFolder#computeChildren} or an out-of-band event handler.
*
* @see ComputedFolder#computeChildren(ChildObserver, TaskListener)
* @see ComputedFolder#createEventsChildObserver()
* @see ComputedFolder#openEventsChildObserver()
*/
public abstract class ChildObserver<I extends TopLevelItem> {
public abstract class ChildObserver<I extends TopLevelItem> implements AutoCloseable {

/** Not implementable outside package. */
ChildObserver() {}
/**
* Not implementable outside package.
*/
ChildObserver() {
}

/**
* Checks whether there is an existing child which should be updated.
* Checks whether there is an existing child which should be updated. It is <strong>stronly </strong>recommended to
* call {@link #completed(String)} after completion of processing the proposed {@link Item#getName()} as otherwise
* no other {@link ChildObserver} will be able to proceed with this {@link Item#getName()}.
*
* @param name a proposed {@link Item#getName}
* @return the existing child to update, if there is one (in which case go ahead and update it as needed); else null, in which case continue by checking {@link #mayCreate}
* @return the existing child to update, if there is one (in which case go ahead and update it as needed); else
* {@code null}, in which case continue by checking {@link #mayCreate}
* @throws InterruptedException if interrupted.
*/
public abstract @CheckForNull I shouldUpdate(String name);
@CheckForNull
public abstract I shouldUpdate(String name) throws InterruptedException;

/**
* Checks whether we may create a new child of the given name.
*
* @param name a proposed {@link Item#getName}
* @return true if you may go ahead and call {@link #created} (though you are not obliged to do so); false if you may not
* @return true if you may go ahead and call {@link #created} (though you are not obliged to do so); {@code false}
* if you may not
*/
public abstract boolean mayCreate(String name);

/**
* Notify the observer that you did create a new child.
* @param child a newly constructed child item; do not call {@link Item#onCreatedFromScratch} and try to avoid calls to {@link Item#save}
*
* @param child a newly constructed child item; do not call {@link Item#onCreatedFromScratch} and try to avoid
* calls to {@link Item#save}
*/
public abstract void created(I child);

/**
* Notify the observer that you have completed with the named child and other threads are now permitted to proceed
* with observations of the {@link Item#getName()}.
*
* @param name the {@link Item#getName()}.
* @since 6.0.0
*/
public abstract void completed(String name);

/**
* Returns a copy of the item names that have been observed.
*
* @return a copy of the item names that have been observed.
* @since FIXME
* @since 5.14
*/
public abstract Set<String> observed();

/**
* Returns a copy of the map of orphaned items keyed by name.
*
* @return a copy of the map of orphaned items keyed by name.
* @since FIXME
* @since 5.14
*/
public abstract Map<String,I> orphaned();
public abstract Map<String, I> orphaned();

/**
* Closes the {@link ChildObserver} completing any observations that were not {@link #completed(String)}.
* This method is idempotent.
*/
@Override
public abstract void close();
}
@@ -64,6 +64,8 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.ReentrantLock;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.CheckForNull;
@@ -72,6 +74,7 @@
import jenkins.model.Jenkins;
import jenkins.model.ParameterizedJobMixIn;
import jenkins.util.TimeDuration;
import net.jcip.annotations.GuardedBy;
import net.sf.json.JSONObject;
import org.acegisecurity.Authentication;
import org.apache.commons.io.FileUtils;
@@ -118,6 +121,36 @@
@Nonnull
private transient FolderComputation<I> computation;

/**
* Lock to guard {@link #currentObservations}.
*
* @since 6.0.0
*/
@Nonnull
private transient final ReentrantLock currentObservationsLock = new ReentrantLock();
/**
* Condition to flag whenever the {@link #currentObservationsChanged} has had elements removed.
*
* @since 6.0.0
*/
private transient final Condition currentObservationsChanged = currentObservationsLock.newCondition();
/**
* The names of the child items that are currently being observed.
*
* @since 6.0.0
*/
@GuardedBy("#computationLock")
private transient final Set<String> currentObservations = new HashSet<>();
/**
* Flag set when the implementation uses {@link #createEventsChildObserver()} and not
* {@link #openEventsChildObserver()}, when {@code true} then the {@link #currentObservations} is ignored
* as we cannot rely on the implementation to call {@link ChildObserver#close()}.
*
* @since 6.0.0
*/
@GuardedBy("#computationLock")
private transient boolean currentObservationsLockDisabled = true;

/**
* Tracks recalculation requirements in {@link #doConfigSubmit(StaplerRequest, StaplerResponse)}.
*
@@ -215,20 +248,21 @@ final void updateChildren(final TaskListener listener) throws IOException, Inter
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "updating {0}", getFullName());
}
FullReindexChildObserver observer = new FullReindexChildObserver();
computeChildren(observer, listener);
Map<String, I> orphaned = observer.orphaned();
if (!orphaned.isEmpty()) {
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "{0}: orphaned {1}",
new Object[]{getFullName(), orphaned.keySet()});
}
for (I existing : orphanedItems(orphaned.values(), listener)) {
try (FullReindexChildObserver observer = new FullReindexChildObserver()) {
computeChildren(observer, listener);
Map<String, I> orphaned = observer.orphaned();
if (!orphaned.isEmpty()) {
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "{0}: deleting {1}", new Object[]{getFullName(), existing});
LOGGER.log(Level.FINE, "{0}: orphaned {1}",
new Object[]{getFullName(), orphaned.keySet()});
}
for (I existing : orphanedItems(orphaned.values(), listener)) {
if (LOGGER.isLoggable(Level.FINE)) {
LOGGER.log(Level.FINE, "{0}: deleting {1}", new Object[]{getFullName(), existing});
}
existing.delete();
// super.onDeleted handles removal from items
}
existing.delete();
// super.onDeleted handles removal from items
}
}
if (LOGGER.isLoggable(Level.FINE)) {
@@ -242,8 +276,36 @@ final void updateChildren(final TaskListener listener) throws IOException, Inter
* which is only applied as part of a full computation.
*
* @return a {@link ChildObserver} for event handling.
* @deprecated use {@link #openEventsChildObserver()}
*/
@Deprecated
@Restricted(NoExternalUse.class) // cause a compilation error to force implementations to switch
protected final ChildObserver<I> createEventsChildObserver() {
LOGGER.log(Level.WARNING, "The {0} implementation of ComputedFolder has not been updated to use "
+ "openEventsChildObserver(), this may result in 'java.lang.IllegalStateException: JENKINS-23152 ... "
+ "already existed; will not overwrite with ...' being thrown when processing events",
getClass().getName());
currentObservationsLock.lock();
try {
if (!currentObservationsLockDisabled) {
currentObservationsLockDisabled = true;
currentObservationsChanged.signalAll();
}
} finally {
currentObservationsLock.unlock();
}
return new EventChildObserver();
}

/**
* Opens a new {@link ChildObserver} that subclasses can use when handling events that might create new / update
* existing child items. The handling of orphaned items is a responsibility of the {@link OrphanedItemStrategy}
* which is only applied as part of a full computation.
*
* @return a {@link ChildObserver} for event handling. The caller must {@link ChildObserver#close()} when done.
* @since 6.0.0
*/
protected final ChildObserver<I> openEventsChildObserver() {
return new EventChildObserver();
}

@@ -642,14 +704,41 @@ public OrphanedItemStrategy getOrphanedItemStrategy() {
private class FullReindexChildObserver extends ChildObserver<I> {
private final Map<String, I> orphaned = new HashMap<String,I>(items);
private final Set<String> observed = new HashSet<String>();
private final Set<String> locked = new HashSet<String>();
private final String fullName = getFullName();

@Override
public void close() {
if (!locked.isEmpty()) {
currentObservationsLock.lock();
try {
currentObservations.removeAll(locked);
currentObservationsChanged.signalAll();
} finally {
currentObservationsLock.unlock();
}
}
}

/**
* {@inheritDoc}
*/
@Override
public I shouldUpdate(String name) {
public I shouldUpdate(String name) throws InterruptedException {
currentObservationsLock.lock();
try {
while (!currentObservations.add(name) && !currentObservationsLockDisabled) {
currentObservationsChanged.await();
}
locked.add(name);
} finally {
currentObservationsLock.unlock();
}
I existing = orphaned.remove(name);
if (existing == null) {
// may have been created by a parallel event
existing = items.get(name);
}
if (existing != null) {
observed.add(name);
}
@@ -664,6 +753,9 @@ public I shouldUpdate(String name) {
public boolean mayCreate(String name) {
boolean r = observed.add(name);
LOGGER.log(Level.FINE, "{0}: may create {1}? {2}", new Object[] {fullName, name, r});
if (!r) {
completed(name);
}
return r;
}

@@ -692,6 +784,25 @@ public void created(I child) {
j.rebuildDependencyGraphAsync();
}
ItemListener.fireOnCreated(child);
// signal this name early
completed(name);
}

/**
* {@inheritDoc}
*/
@Override
public void completed(String name) {
if (locked.contains(name)) {
currentObservationsLock.lock();
try {
locked.remove(name);
currentObservations.remove(name);
currentObservationsChanged.signalAll();
} finally {
currentObservationsLock.unlock();
}
}
}

/**
@@ -714,12 +825,35 @@ public void created(I child) {
private class EventChildObserver extends ChildObserver<I> {
private final String fullName = getFullName();
private final Set<String> observed = new HashSet<String>();
private final Set<String> locked = new HashSet<String>();

@Override
public void close() {
if (!locked.isEmpty()) {
currentObservationsLock.lock();
try {
currentObservations.removeAll(locked);
currentObservationsChanged.signalAll();
} finally {
currentObservationsLock.unlock();
}
}
}

/**
* {@inheritDoc}
*/
@Override
public I shouldUpdate(String name) {
public I shouldUpdate(String name) throws InterruptedException {
currentObservationsLock.lock();
try {
while (!currentObservations.add(name) && !currentObservationsLockDisabled) {
currentObservationsChanged.await();
}
locked.add(name);
} finally {
currentObservationsLock.unlock();
}
I existing = items.get(name);
if (existing != null) {
observed.add(name);
@@ -735,6 +869,9 @@ public I shouldUpdate(String name) {
public boolean mayCreate(String name) {
boolean r = !items.containsKey(name) && observed.add(name);
LOGGER.log(Level.FINE, "{0}: may create {1}? {2}", new Object[]{fullName, name, r});
if (!r) {
completed(name);
}
return r;
}

@@ -767,6 +904,25 @@ public void created(I child) {
j.rebuildDependencyGraphAsync();
}
ItemListener.fireOnCreated(child);
// signal this name early
completed(name);
}

/**
* {@inheritDoc}
*/
@Override
public void completed(String name) {
if (locked.contains(name)) {
currentObservationsLock.lock();
try {
locked.remove(name);
currentObservations.remove(name);
currentObservationsChanged.signalAll();
} finally {
currentObservationsLock.unlock();
}
}
}

/**
@@ -171,6 +171,9 @@ public String getInterval() {
*/
@Override
public void run() {
if (job == null) {
return;
}
long now = System.currentTimeMillis();
FolderComputation<?> computation = job.getComputation();
if (computation != null) {

0 comments on commit 0d8d280

Please sign in to comment.
You can’t perform that action at this time.