Skip to content
Permalink
Browse files
[JENKINS-47714] - Introduce SerializableOnlyOverRemoting and cleanup …
…FindBugs in Channel#current(). (#206)

* [JENKINS-47714] - Introduce SerializableOnlyOverRemoting and cleanup FindBugs in Channel#current().

`Channel#current()` uses thread-local storage to determine the current channel. It returns null if the channel does not exist Some writeReplace/readObject/etc. serialization logic retrieves the channel in order to export the object via ExportTable. Obviously, such operations will fail if we try to serialize the object without Remoting context.
I propose to add a new interface to verify that serialization logic is being invoked for the remoting context and hence to avoid undesired NPEs.

* [JENKINS-47714] - Address comments from @jglick

* [JENKINS-47714] - Add @SInCE to new API
  • Loading branch information
oleg-nenashev committed Nov 8, 2017
1 parent b46db64 commit d906a333f34860dbf3a3f0771b1c1dfd1e071ca6
@@ -190,7 +190,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.kohsuke</groupId>
<artifactId>access-modifier-annotation</artifactId>
<version>1.7</version>
<version>1.12</version>
<type>jar</type>
<optional>true</optional><!-- no need to have this at runtime -->
</dependency>
@@ -27,6 +27,7 @@

import java.io.Serializable;

//TODO: Make it SerializableOnlyOverRemoting?
/**
* Represents computation to be done on a remote system.
*
@@ -1175,7 +1175,7 @@ public void setMaximumBytecodeLevel(short level) throws IOException, Interrupted
this.level = level;
}
public Void call() throws RuntimeException {
Channel.current().maximumBytecodeLevel = level;
Channel.currentOrFail().maximumBytecodeLevel = level;
return null;
}

@@ -1630,8 +1630,9 @@ public void run() {
}

private static final class IOSyncer implements Callable<Object, InterruptedException> {
@Override
public Object call() throws InterruptedException {
Channel.current().syncLocalIO();
Channel.currentOrFail().syncLocalIO();
return null;
}

@@ -1716,6 +1717,24 @@ public static Channel current() {
return CURRENT.get();
}

/**
* Gets current channel or fails with {@link IllegalStateException}.
*
* @return Current channel
* @throws IllegalStateException the calling thread has no associated channel.
* @since 3.14
* @see org.jenkinsci.remoting.SerializableOnlyOverRemoting
*/
@Nonnull
public static Channel currentOrFail() throws IllegalStateException {
final Channel ch = CURRENT.get();
if (ch == null) {
final Thread t = Thread.currentThread();
throw new IllegalStateException("The calling thread " + t + " has no associated channel");
}
return ch;
}

// TODO: Unrestrict after the merge into the master.
// By now one may use it via the reflection logic only
/**
@@ -1,11 +1,11 @@
package hudson.remoting;

import hudson.remoting.RemoteClassLoader.IClassLoader;
import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import javax.annotation.CheckForNull;

/**
@@ -14,7 +14,7 @@
* @author Kohsuke Kawaguchi
* @since 2.12
*/
public class ClassLoaderHolder implements Serializable {
public class ClassLoaderHolder implements SerializableOnlyOverRemoting {

@CheckForNull
private transient ClassLoader classLoader;
@@ -37,14 +37,14 @@ public void set(@CheckForNull ClassLoader classLoader) {

private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
IClassLoader proxy = (IClassLoader)ois.readObject();
classLoader = proxy==null ? null : Channel.current().importedClassLoaders.get(proxy);
classLoader = proxy==null ? null : getChannelForSerialization().importedClassLoaders.get(proxy);
}

private void writeObject(ObjectOutputStream oos) throws IOException {
if (classLoader==null)
oos.writeObject(null);
else {
IClassLoader proxy = RemoteClassLoader.export(classLoader, Channel.current());
IClassLoader proxy = RemoteClassLoader.export(classLoader, getChannelForSerialization());
oos.writeObject(proxy);
}
}
@@ -1,9 +1,11 @@
package hudson.remoting;

import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

import java.io.File;
import java.io.IOException;
import java.io.NotSerializableException;
import java.io.OutputStream;
import java.io.Serializable;
import java.net.URL;
import java.util.Collections;
import java.util.HashSet;
@@ -17,7 +19,7 @@
* @author Kohsuke Kawaguchi
*/
@edu.umd.cs.findbugs.annotations.SuppressWarnings("SE_BAD_FIELD")
class JarLoaderImpl implements JarLoader, Serializable {
class JarLoaderImpl implements JarLoader, SerializableOnlyOverRemoting {
private final ConcurrentMap<Checksum,URL> knownJars = new ConcurrentHashMap<>();

@edu.umd.cs.findbugs.annotations.SuppressWarnings("DMI_COLLECTION_OF_URLS") // TODO: fix this
@@ -71,8 +73,8 @@ public Checksum calcChecksum(URL jar) throws IOException {
/**
* When sent to the remote node, send a proxy.
*/
private Object writeReplace() {
return Channel.current().export(JarLoader.class, this);
private Object writeReplace() throws NotSerializableException {
return getChannelForSerialization().export(JarLoader.class, this);
}

public static final String DIGEST_ALGORITHM = System.getProperty(JarLoaderImpl.class.getName()+".algorithm","SHA-256");
@@ -23,12 +23,13 @@
*/
package hudson.remoting;

import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.OutputStream;
import java.io.Serializable;
import java.util.concurrent.ExecutionException;
import java.util.logging.Level;
import java.util.logging.Logger;
@@ -96,7 +97,7 @@
*
* @author Kohsuke Kawaguchi
*/
public final class Pipe implements Serializable, ErrorPropagatingOutputStream {
public final class Pipe implements SerializableOnlyOverRemoting, ErrorPropagatingOutputStream {
private InputStream in;
private OutputStream out;

@@ -149,30 +150,31 @@ public static Pipe createLocalToRemote() {
}

private void writeObject(ObjectOutputStream oos) throws IOException {
final Channel ch = getChannelForSerialization();

// TODO: there's a discrepancy in the pipe window size and FastPipedInputStream buffer size.
// The former uses 1M, while the latter uses 64K, so if the sender is too fast, it'll cause
// the pipe IO thread to block other IO activities. Fix this by first using adaptive growing buffer
// in FastPipedInputStream, then make sure the maximum size is biger than the pipe window size.
if(in!=null && out==null) {
// remote will write to local
FastPipedOutputStream pos = new FastPipedOutputStream((FastPipedInputStream)in);
int oid = Channel.current().internalExport(Object.class, pos, false); // this export is unexported in ProxyOutputStream.finalize()
int oid = ch.internalExport(Object.class, pos, false); // this export is unexported in ProxyOutputStream.finalize()

oos.writeBoolean(true); // marker
oos.writeInt(oid);
} else {
// remote will read from local this object gets unexported when the pipe is connected.
// see ConnectCommand
int oid = Channel.current().internalExport(Object.class, out, false);
int oid = ch.internalExport(Object.class, out, false);

oos.writeBoolean(false);
oos.writeInt(oid);
}
}

private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
final Channel channel = Channel.current();
assert channel !=null;
final Channel channel = getChannelForSerialization();

if(ois.readBoolean()) {
// local will write to remote
@@ -23,6 +23,7 @@
*/
package hudson.remoting;

import javax.annotation.Nonnull;
import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
@@ -67,14 +68,14 @@ public ProxyOutputStream() {
* @param oid
* The object id of the exported {@link OutputStream}.
*/
public ProxyOutputStream(Channel channel, int oid) throws IOException {
public ProxyOutputStream(@Nonnull Channel channel, int oid) throws IOException {
connect(channel,oid);
}

/**
* Connects this stream to the specified remote object.
*/
synchronized void connect(Channel channel, int oid) throws IOException {
synchronized void connect(@Nonnull Channel channel, int oid) throws IOException {
if(this.channel!=null)
throw new IllegalStateException("Cannot connect twice");
if(oid==0)
@@ -27,6 +27,7 @@
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.ObjectStreamException;
import java.io.Serializable;
import java.lang.reflect.Method;
import java.net.URL;
@@ -46,6 +47,7 @@
import java.util.logging.Logger;

import org.jenkinsci.constant_pool_scanner.ConstantPoolScanner;
import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

import javax.annotation.CheckForNull;

@@ -1034,7 +1036,7 @@ public String toString() {
* to work (which will be the remote instance.) Once transferred to the other side,
* resolve back to the instance on the server.
*/
private static class RemoteIClassLoader implements IClassLoader, Serializable {
private static class RemoteIClassLoader implements IClassLoader, SerializableOnlyOverRemoting {
private transient final IClassLoader proxy;
private final int oid;

@@ -1079,9 +1081,9 @@ public ResourceFile getResource2(String name) throws IOException {
return proxy.getResources2(name);
}

private Object readResolve() {
private Object readResolve() throws ObjectStreamException {
try {
return Channel.current().getExportedObject(oid);
return getChannelForSerialization().getExportedObject(oid);
} catch (ExecutionException ex) {
//TODO: Implement something better?
throw new IllegalStateException("Cannot resolve remoting classloader", ex);
@@ -23,11 +23,12 @@
*/
package hudson.remoting;

import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

import java.io.BufferedInputStream;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.io.Serializable;
import java.io.ObjectOutputStream;
import java.io.IOException;
import java.io.ObjectInputStream;
@@ -47,7 +48,7 @@
*
* @author Kohsuke Kawaguchi
*/
public class RemoteInputStream extends InputStream implements Serializable {
public class RemoteInputStream extends InputStream implements SerializableOnlyOverRemoting {
private static final Logger LOGGER = Logger.getLogger(RemoteInputStream.class.getName());
private transient InputStream core;
private boolean autoUnexport;
@@ -107,7 +108,7 @@ public RemoteInputStream(InputStream core, Set<Flag> flags) {
}

private void writeObject(ObjectOutputStream oos) throws IOException {
Channel ch = Channel.current();
final Channel ch = getChannelForSerialization();
if (ch.remoteCapability.supportsGreedyRemoteInputStream()) {
oos.writeBoolean(greedy);

@@ -175,9 +176,7 @@ public void run() {
}

private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
final Channel channel = Channel.current();
assert channel !=null;

final Channel channel = getChannelForSerialization();
if (channel.remoteCapability.supportsGreedyRemoteInputStream()) {
boolean greedy = ois.readBoolean();
if (greedy) {
@@ -53,6 +53,7 @@
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.jenkinsci.remoting.RoleChecker;

import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

@@ -61,6 +62,8 @@
*
* @author Kohsuke Kawaguchi
*/
//TODO: Likely should be serializable over Remoting logic, but this class has protection logic
// Use-cases need to be investigated
final class RemoteInvocationHandler implements InvocationHandler, Serializable {
/**
* Our logger.
@@ -246,7 +249,7 @@ public Object invoke(Object proxy, Method method, Object[] args) throws Throwabl
if(method.getDeclaringClass()==IReadResolve.class) {
// readResolve on the proxy.
// if we are going back to where we came from, replace the proxy by the real object
if(goingHome) return Channel.current().getExportedObject(oid);
if(goingHome) return Channel.currentOrFail().getExportedObject(oid);
else return proxy;
}

@@ -887,7 +890,7 @@ public RPCRequest(int oid, Method m, Object[] arguments, ClassLoader cl) {
}

public Serializable call() throws Throwable {
return perform(Channel.current());
return perform(Channel.currentOrFail());
}

@Override
@@ -903,7 +906,7 @@ public ClassLoader getClassLoader() {
return getClass().getClassLoader();
}

protected Serializable perform(Channel channel) throws Throwable {
protected Serializable perform(@Nonnull Channel channel) throws Throwable {
Object o = channel.getExportedObject(oid);
Class[] clazz = channel.getExportedTypes(oid);
try {
@@ -23,11 +23,12 @@
*/
package hudson.remoting;

import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.OutputStream;
import java.io.Serializable;

/**
* {@link OutputStream} that can be sent over to the remote {@link Channel},
@@ -62,7 +63,7 @@
* @see RemoteInputStream
* @author Kohsuke Kawaguchi
*/
public final class RemoteOutputStream extends OutputStream implements Serializable {
public final class RemoteOutputStream extends OutputStream implements SerializableOnlyOverRemoting {
/**
* On local machine, this points to the {@link OutputStream} where
* the data will be sent ultimately.
@@ -79,15 +80,12 @@ public RemoteOutputStream(OutputStream core) {
}

private void writeObject(ObjectOutputStream oos) throws IOException {
int id = Channel.current().internalExport(OutputStream.class, core, false); // this export is unexported in ProxyOutputStream.finalize()
int id = getChannelForSerialization().internalExport(OutputStream.class, core, false); // this export is unexported in ProxyOutputStream.finalize()
oos.writeInt(id);
}

private void readObject(ObjectInputStream ois) throws IOException, ClassNotFoundException {
final Channel channel = Channel.current();
assert channel !=null;

this.core = new ProxyOutputStream(channel, ois.readInt());
this.core = new ProxyOutputStream(getChannelForSerialization(), ois.readInt());
}

private static final long serialVersionUID = 1L;
Loading

0 comments on commit d906a33

Please sign in to comment.