Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Document and annotate object Export/Unexport logic
During last releases we have fixed several issues related to the object export/unexport operations (JENKINS-23271, JENKINS-41852). I decided that it makes sense to improve the method documentation just to make Javadocs more explicit.

The change also adds missing documentation of hudson.remoting.ExportTable.unexportLogSize
  • Loading branch information
oleg-nenashev committed Feb 12, 2017
1 parent cb45f65 commit 27bb51d
Show file tree
Hide file tree
Showing 8 changed files with 100 additions and 29 deletions.
8 changes: 8 additions & 0 deletions docs/configuration.md
Expand Up @@ -82,6 +82,14 @@ These properties require independent configuration on both sides of the channel.
<code>SocketTimeoutException</code>
</td>
</tr>
<tr>
<td>hudson.remoting.ExportTable.unexportLogSize</td>
<td>1024</td>
<td>2.40</td>
<td>?</td>
<td><a href="https://issues.jenkins-ci.org/browse/JENKINS-20707">JENKINS-20707</a></td>
<td>Defines number of entries to be stored in the unexport history, which is being analyzed during the invalid object ID analysis.</td>
</tr>
<tr>
<td>${PROTOCOL_FULLY_QUALIFIED_NAME}.disabled,
where PROTOCOL_FULLY_QUALIFIED_NAME equals
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/hudson/remoting/Base64.java
Expand Up @@ -17,6 +17,7 @@

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import javax.annotation.CheckForNull;
import javax.annotation.Nullable;

/**
* This class provides encode/decode for RFC 2045 Base64 as
Expand Down Expand Up @@ -101,8 +102,9 @@ protected static boolean isBase64(char octect) {
* Encodes hex octects into Base64
*
* @param binaryData Array containing binaryData
* @return Encoded Base64 array
* @return Encoded Base64 array. {@code null} if the input is null
*/
@Nullable
public static String encode(byte[] binaryData) {

if (binaryData == null)
Expand Down
16 changes: 12 additions & 4 deletions src/main/java/hudson/remoting/Channel.java
Expand Up @@ -60,6 +60,7 @@
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import javax.annotation.Nullable;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

Expand Down Expand Up @@ -613,11 +614,13 @@ private ExecutorService createPipeWriterExecutor() {
/**
* {@inheritDoc}
*/
@Override
public <T> T export(Class<T> type, T instance) {
return export(type, instance, true);
}

/**
* Exports the class instance to the remote side.
* @param userProxy
* If true, the returned proxy will be capable to handle classes
* defined in the user classloader as parameters and return values.
Expand All @@ -626,10 +629,14 @@ public <T> T export(Class<T> type, T instance) {
* used by {@link RemoteClassLoader}.
*
* To create proxies for objects inside remoting, pass in false.
* @return Reference to the exported instance wrapped by {@link RemoteInvocationHandler}.
* {@code null} if the input instance is {@code null}.
*/
/*package*/ <T> T export(Class<T> type, T instance, boolean userProxy) {
if(instance==null)
@Nullable
/*package*/ <T> T export(Class<T> type, @CheckForNull T instance, boolean userProxy) {
if(instance==null) {
return null;
}

// every so often perform GC on the remote system so that
// unused RemoteInvocationHandler get released, which triggers
Expand Down Expand Up @@ -686,8 +693,9 @@ public <T> T export(Class<T> type, T instance) {

/**
* Increase reference count so much to effectively prevent de-allocation.
* @param instance Instance to be pinned
*/
public void pin(Object instance) {
public void pin(@Nonnull Object instance) {
exportedObjects.pin(instance);
}

Expand Down Expand Up @@ -1089,7 +1097,7 @@ public void checkRoles(RoleChecker checker) throws SecurityException {

/**
* Waits for this {@link Channel} to be closed down, but only up the given milliseconds.
*
* @param timeout TImeout in milliseconds
* @throws InterruptedException
* If the current thread is interrupted while waiting for the completion.
* @since 1.299
Expand Down
Expand Up @@ -3,6 +3,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import javax.annotation.CheckForNull;

/**
* {@link OutputStream} that's connected to an {@link InputStream} somewhere,
Expand Down Expand Up @@ -39,5 +40,5 @@ public interface ErrorPropagatingOutputStream {
* @param e
* if null, this method behaves exactly like {@link OutputStream#close()}
*/
void error(Throwable e) throws IOException;
void error(@CheckForNull Throwable e) throws IOException;
}
75 changes: 56 additions & 19 deletions src/main/java/hudson/remoting/ExportTable.java
Expand Up @@ -73,14 +73,17 @@ private final class Entry<T> {
* {@code object.getClass().getName()} kept around so that we can see the type even after it
* gets deallocated.
*/
@Nonnull
private final String objectType;
/**
* Where was this object first exported?
*/
@Nonnull
final CreatedAt allocationTrace;
/**
* Where was this object unexported?
*/
@CheckForNull
ReleasedAt releaseTrace;
/**
* Current reference count.
Expand All @@ -92,9 +95,10 @@ private final class Entry<T> {
/**
* This field can be set programmatically to track reference counting
*/
@CheckForNull
private ReferenceCountRecorder recorder;

Entry(T object, Class<? super T>... interfaces) {
Entry(@Nonnull T object, Class<? super T>... interfaces) {
this.id = iota++;
this.interfaces = interfaces.clone();
this.object = object;
Expand Down Expand Up @@ -122,7 +126,13 @@ void pin() {
referenceCount += Integer.MAX_VALUE/2;
}

void release(Throwable callSite) {
/**
* Releases the entry.
* @param callSite
* Optional location that indicates where the actual call site was that triggered the activity,
* in case it was requested from the other side of the channel.
*/
void release(@CheckForNull Throwable callSite) {
if (recorder!=null)
recorder.onRelease(callSite);

Expand Down Expand Up @@ -204,7 +214,7 @@ static class Source extends Exception {
* Optional location that indicates where the actual call site was that triggered the activity,
* in case it was requested from the other side of the channel.
*/
Source(Throwable callSite) {
Source(@CheckForNull Throwable callSite) {
super(callSite);
// force the computation of the stack trace in a Java friendly data structure,
// so that the call stack can be seen from the heap dump after the fact.
Expand All @@ -217,16 +227,18 @@ static class CreatedAt extends Source {
super(null);
}

@Override
public String toString() {
return " Created at "+new Date(timestamp);
}
}

static class ReleasedAt extends Source {
ReleasedAt(Throwable callSite) {
ReleasedAt(@CheckForNull Throwable callSite) {
super(callSite);
}

@Override
public String toString() {
return " Released at "+new Date(timestamp);
}
Expand Down Expand Up @@ -288,19 +300,26 @@ boolean isRecording() {
* @return
* The assigned 'object ID'. If the object is already exported,
* it will return the ID already assigned to it.
* @param clazz
* @param t
* {@code 0} if the input parameter is {@code null}.
* @param clazz Class of the object
* @param t Class instance
*/
synchronized <T> int export(Class<T> clazz, T t) {
synchronized <T> int export(@Nonnull Class<T> clazz, @CheckForNull T t) {
return export(clazz, t,true);
}

/**
* @param clazz
* Exports the given object.
* @param clazz Class of the object
* @param t Object to be exported
* @param notifyListener
* If false, listener will not be notified. This is used to
* @return
* The assigned 'object ID'. If the object is already exported,
* it will return the ID already assigned to it.
* {@code 0} if the input parameter is {@code null}.
*/
synchronized <T> int export(Class<T> clazz, T t, boolean notifyListener) {
synchronized <T> int export(@Nonnull Class<T> clazz, @CheckForNull T t, boolean notifyListener) {
if(t==null) return 0; // bootstrap classloader

Entry e = reverse.get(t);
Expand All @@ -319,14 +338,21 @@ synchronized <T> int export(Class<T> clazz, T t, boolean notifyListener) {
return e.id;
}

/*package*/ synchronized void pin(Object t) {
/*package*/ synchronized void pin(@Nonnull Object t) {
Entry e = reverse.get(t);
if(e!=null)
e.pin();
}

synchronized @Nonnull
Object get(int id) throws ExecutionException {
/**
* Retrieves object by id.
* @param id Object ID
* @return Object
* @throws ExecutionException The requested ID cannot be found.
* The root cause will be diagnosed by {@link #diagnoseInvalidObjectId(int)}.
*/
@Nonnull
synchronized Object get(int id) throws ExecutionException {
Entry e = table.get(id);
if(e!=null) return e.object;

Expand All @@ -347,8 +373,8 @@ synchronized Object getOrNull(int oid) {
return null;
}

synchronized @Nonnull
Class[] type(int id) throws ExecutionException {
@Nonnull
synchronized Class[] type(int id) throws ExecutionException {
Entry e = table.get(id);
if(e!=null) return e.getInterfaces();

Expand All @@ -362,8 +388,11 @@ Class[] type(int id) throws ExecutionException {
* Exported {@link Pipe}s are vulnerable to infinite blocking
* when the channel is lost and the sender side is cut off. The reader
* end will not see that the writer has disappeared.
*
* @param e Termination error
*
*/
void abort(Throwable e) {
void abort(@CheckForNull Throwable e) {
List<Entry<?>> values;
synchronized (this) {
values = new ArrayList<Entry<?>>(table.values());
Expand Down Expand Up @@ -392,6 +421,7 @@ void abort(Throwable e) {
* @param id Object ID
* @return Exception to be thrown
*/
@Nonnull
private synchronized ExecutionException diagnoseInvalidObjectId(int id) {
Exception cause=null;

Expand All @@ -410,8 +440,10 @@ private synchronized ExecutionException diagnoseInvalidObjectId(int id) {

/**
* Removes the exported object from the table.
* @param t Object to be unexported. {@code null} instances will be ignored.
* @param callSite Stacktrace of the invocation source
*/
synchronized void unexport(Object t, Throwable callSite) {
synchronized void unexport(@CheckForNull Object t, Throwable callSite) {
if(t==null) return;
Entry e = reverse.get(t);
if(e==null) {
Expand All @@ -431,12 +463,12 @@ void unexportByOid(Integer oid, Throwable callSite) {

/**
* Removes the exported object for the specified oid from the table.
* @param oid Object ID
* @param oid Object ID. If {@code null} the method will do nothing.
* @param callSite Unexport command caller
* @param severeErrorIfMissing Consider missing object as {@link #SEVERE} error. {@link #FINE} otherwise
* @since TODO
*/
synchronized void unexportByOid(Integer oid, Throwable callSite, boolean severeErrorIfMissing) {
synchronized void unexportByOid(@CheckForNull Integer oid, @CheckForNull Throwable callSite, boolean severeErrorIfMissing) {
if(oid==null) return;
Entry e = table.get(oid);
if(e==null) {
Expand All @@ -451,8 +483,9 @@ synchronized void unexportByOid(Integer oid, Throwable callSite, boolean severeE

/**
* Dumps the contents of the table to a file.
* @throws IOException Output error
*/
synchronized void dump(PrintWriter w) throws IOException {
synchronized void dump(@Nonnull PrintWriter w) throws IOException {
for (Entry e : table.values()) {
e.dump(w);
}
Expand All @@ -462,6 +495,10 @@ synchronized void dump(PrintWriter w) throws IOException {
return reverse.containsKey(o);
}

/**
* Defines number of entries to be stored in the unexport history.
* @since 2.40
*/
public static int UNEXPORT_LOG_SIZE = Integer.getInteger(ExportTable.class.getName()+".unexportLogSize",1024);

private static final Logger LOGGER = Logger.getLogger(ExportTable.class.getName());
Expand Down
1 change: 1 addition & 0 deletions src/main/java/hudson/remoting/LocalChannel.java
Expand Up @@ -91,6 +91,7 @@ public void join() throws InterruptedException {
// noop
}

@Override
public void join(long timeout) throws InterruptedException {
// noop
}
Expand Down
1 change: 1 addition & 0 deletions src/main/java/hudson/remoting/RemoteInvocationHandler.java
Expand Up @@ -130,6 +130,7 @@ final class RemoteInvocationHandler implements InvocationHandler, Serializable {
/**
* Wraps an OID to the typed wrapper.
*/
@Nonnull
public static <T> T wrap(Channel channel, int id, Class<T> type, boolean userProxy, boolean autoUnexportByCaller) {
ClassLoader cl = type.getClassLoader();
// if the type is a JDK-defined type, classloader should be for IReadResolve
Expand Down
21 changes: 17 additions & 4 deletions src/main/java/hudson/remoting/VirtualChannel.java
Expand Up @@ -24,6 +24,9 @@
package hudson.remoting;

import java.io.IOException;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;

/**
* Virtualized {@link Channel} that allows different implementations.
Expand Down Expand Up @@ -81,7 +84,7 @@ public interface VirtualChannel {

/**
* Waits for this {@link Channel} to be closed down, but only up the given milliseconds.
*
* @param timeout Timeout in milliseconds
* @throws InterruptedException
* If the current thread is interrupted while waiting for the completion.
* @since 1.300
Expand All @@ -91,18 +94,28 @@ public interface VirtualChannel {
/**
* Exports an object for remoting to the other {@link Channel}
* by creating a remotable proxy.
*
* The returned reference must be kept if there is ongoing operation on the remote side.
* Once it is released, the exported object will be deallocated as well.
* Please keep in mind that the object may be also released earlier than expected by JVM
* (e.g. see <a href="https://issues.jenkins-ci.org/browse/JENKINS-23271">JENKINS-23271</a>).
*
* @param instance
* Instance to be exported.
* {@code null} instances won't be exported to the remote instance.
* <p>
* All the parameters and return values must be serializable.
*
* @param <T>
* Type
* @param type
* Interface to be remoted.
* @return
* the proxy object that implements <tt>T</tt>. This object can be transfered
* to the other {@link Channel}, and calling methods on it from the remote side
* will invoke the same method on the given local <tt>instance</tt> object.
* {@code null} if the input instance is {@code null}.
*/
<T> T export( Class<T> type, T instance);
@Nullable
<T> T export(@Nonnull Class<T> type, @CheckForNull T instance);

/**
* Blocks until all the I/O packets sent from remote is fully locally executed, then return.
Expand Down

0 comments on commit 27bb51d

Please sign in to comment.