Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JENKINS-37566] - FindBugs Cleanup. Part #1 (#109)
* [JENKINS-37566] - FindBugs: Unclosed stream in hudson.remoting.Capability

* [JENKINS-37566] - FindBugs: Stream not closed on Exception path in InitializeJarCacheMain#copyFile

* [JENKINS-37566] - Fix Util#makeResource and remove obsolete hack

* Extra Util#makeResource() polishing

* Deprecate obsolete Util#mkdirs()

* Exceptional case in setLastModifiedTime

* Handle exception case during temp file deletion in FileSystemJarCache

* Synchronize ProxyWriter#closed in write()

* Synchronization of ProxyWriter#channel

* Get rid of the obsolete collection in Channel#properties, fix synchronization

* Checksum#calculateFor() - no need to put if absent since there is a check above

* UWF_UNWRITTEN_FIELD in ExportTable$Entry

* NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE in JnlpAgentEndpointResolver.
Now we use a more agressive check

* UWF_FIELD_NOT_INITIALIZED_IN_CONSTRUCTOR in org.jenkinsci.remoting.nio.NioChannelHub$NioTransport.abort

* UPM_UNCALLED_PRIVATE_METHOD - hudson.remoting.ChunkedOutputStream.frameSize()

* FindBugs: Revert the stream closure at emoting Capability, just a design to be reworked

* Unrealistic NP_NULL_ON_SOME_PATH in NioChannelHub

* EI_EXPOSE_REP in DiagnosedStreamCorruptionException (diagnostics code)

* SE_TRANSIENT_FIELD_NOT_RESTORED in PreloadJarTask, RemoteClassLoader, RemoteInvocationHandler and UserRequest

* Leftover false-positive UG_SYNC_SET_UNSYNC_GET in Channel

* EI_EXPOSE_REP in ChannelBuilder#properties

* Suppress DMI_NONSERIALIZABLE_OBJECT_WRITTEN in ClassLoaderHolder#writeObject()

* Better handling of Streams in hudson.remoting.Capability (still weird)

* Fix the Util#makeResource() behavior for nested folders

* FileSystemJarCache: tmp file may be renamed

* JENKINS-37566 - Modification of Channel#dumpDiagnostics() actually is not required after the merge of #122

* Disable DP_DO_INSIDE_DO_PRIVILEGED as per feeback from @kohsue in #118

* Fix the FindBugs filter

* [NP_NULL_ON_SOME_PATH] - Prevent NPE in ExportTable#diagnoseInvalidObjectId()

* [RV_RETURN_VALUE_IGNORED_BAD_PRACTICE] - Propagate exceptions when FileSystemJarCache#retrieve() fails to retrieve the target

* [DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED] - Ignore the warning as per discussion in #118

* [VO_VOLATILE_INCREMENT] - Suppress warning in Channel#send()

* FindBugs - Which#jarFile() should not suppress all exceptions

* FindBugs - Suppress URF_UNREAD_FIELD in Pipewindow#Real

* [JENKINS-37566] - Channel#properties should be a ConcurrentHashMap

Reason - prevent possible delays and deadlocks in the getter method.
Addresses the feedback from @stephenc

* [JENKINS-37566] - DiagnosedStreamCorruptionException readAhead/readBack handlers should use a more efficient clone() command

* [JENKINS-37566] - Suppress UG_SYNC_SET_UNSYNC_GET after switching to the ConcurrentHashMap

* [JENKINS-37566] - hudson.remoting.Util should be tolerant against InvalidPathException

* [JENKINS-37566] - make ProxyWriter more asynchronous (follow-up to @stephenc’s review)

* [JENKINS-37566] - Add a comment about race condition as suggested by @stephenc
  • Loading branch information
oleg-nenashev committed Nov 8, 2017
1 parent d906a33 commit 49c67ee
Show file tree
Hide file tree
Showing 21 changed files with 227 additions and 123 deletions.
16 changes: 11 additions & 5 deletions src/findbugs/excludeFilter.xml
Expand Up @@ -8,13 +8,19 @@
</Match>

<Match>
<Or>
<!--We do not want do break API by converting potentially usefull classes-->
<Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON"/>
</Or>
<!--We do not want do break API by converting potentially usefull classes-->
<Bug pattern="SIC_INNER_SHOULD_BE_STATIC_ANON"/>
</Match>


<Match>
<!-- TODO: On hold due to the discussion in PR #118 -->
<Bug pattern="DP_DO_INSIDE_DO_PRIVILEGED"/>
</Match>
<Match>
<!-- TODO: On hold due to the discussion in PR #118 -->
<Bug pattern="DP_CREATE_CLASSLOADER_INSIDE_DO_PRIVILEGED"/>
</Match>

<!--TODO: reconsider commented filters after cleanup-->
<!-- ignore all low-level (priority=2,3) issues except serialization problems -->
<!--<Match>
Expand Down
Expand Up @@ -6,6 +6,7 @@
import java.io.ObjectOutputStream;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nonnull;

/**
* {@link CommandTransport} that works with {@code byte[]} instead of command object.
Expand Down Expand Up @@ -35,7 +36,7 @@ public abstract class AbstractByteArrayCommandTransport extends CommandTransport
*
* In this subtype, we pass in {@link ByteArrayReceiver} that uses byte[] instead of {@link Command}
*/
public abstract void setup(ByteArrayReceiver receiver);
public abstract void setup(@Nonnull ByteArrayReceiver receiver);

public static interface ByteArrayReceiver {
/**
Expand Down
26 changes: 20 additions & 6 deletions src/main/java/hudson/remoting/Capability.java
@@ -1,5 +1,6 @@
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.remoting.Channel.Mode;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -108,22 +109,30 @@ public boolean supportsProxyWriter2_35() {
return (mask & MASK_PROXY_WRITER_2_35) != 0;
}

//TODO: ideally preamble handling needs to be reworked in order to avoid FB suppression
/**
* Writes out the capacity preamble.
*/
void writePreamble(OutputStream os) throws IOException {
os.write(PREAMBLE);
ObjectOutputStream oos = new ObjectOutputStream(Mode.TEXT.wrap(os));
oos.writeObject(this);
oos.flush();
try (ObjectOutputStream oos = new ObjectOutputStream(Mode.TEXT.wrap(os)) {
@Override
public void close() throws IOException {
flush();
// TODO: Cannot invoke the private clear() method, but GC well do it for us. Not worse than the original solution
// Here the code does not close the proxied stream OS on completion
}
}) {
oos.writeObject(this);
oos.flush();
}
}

/**
* The opposite operation of {@link #writePreamble(OutputStream)}.
*/
public static Capability read(InputStream is) throws IOException {
try {
ObjectInputStream ois = new ObjectInputStream(Mode.TEXT.wrap(is)) {
try (ObjectInputStream ois = new ObjectInputStream(Mode.TEXT.wrap(is)) {
// during deserialization, only accept Capability to protect ourselves
// from malicious payload. Allow java.lang.String so that
// future versions of Capability can send more complex data structure.
Expand All @@ -136,7 +145,12 @@ protected Class<?> resolveClass(ObjectStreamClass desc) throws IOException, Clas
return super.resolveClass(desc);
throw new SecurityException("Rejected: "+n);
}
};

@Override
public void close() throws IOException {
// Do not close the stream since we continue reading from the input stream "is"
}
}) {
return (Capability)ois.readObject();
} catch (ClassNotFoundException e) {
throw (Error)new NoClassDefFoundError(e.getMessage()).initCause(e);
Expand Down
23 changes: 18 additions & 5 deletions src/main/java/hudson/remoting/Channel.java
Expand Up @@ -23,6 +23,7 @@
*/
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import edu.umd.cs.findbugs.annotations.SuppressWarnings;
import hudson.remoting.CommandTransport.CommandReceiver;
import hudson.remoting.PipeWindow.Key;
Expand All @@ -46,12 +47,14 @@
import java.lang.ref.WeakReference;
import java.net.URL;
import java.util.Collections;
import java.util.HashMap;
import java.util.Date;
import java.util.Hashtable;
import java.util.Locale;
import java.util.Map;
import java.util.Vector;
import java.util.WeakHashMap;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -291,7 +294,7 @@ public class Channel implements VirtualChannel, IChannel, Closeable {
/**
* Property bag that contains application-specific stuff.
*/
private final Hashtable<Object,Object> properties = new Hashtable<Object,Object>();
private final ConcurrentHashMap<Object,Object> properties = new ConcurrentHashMap<>();

/**
* Proxy to the remote {@link Channel} object.
Expand Down Expand Up @@ -662,6 +665,7 @@ private ExecutorService createPipeWriterExecutor() {
* This is the lowest layer of abstraction in {@link Channel}.
* {@link Command}s are executed on a remote system in the order they are sent.
*/
@SuppressFBWarnings(value = "VO_VOLATILE_INCREMENT", justification = "The method is synchronized, no other usages. See https://sourceforge.net/p/findbugs/bugs/1032/")
/*package*/ synchronized void send(Command cmd) throws IOException {
if(outClosed!=null)
throw new ChannelClosedException(outClosed);
Expand Down Expand Up @@ -1343,8 +1347,7 @@ public void dumpDiagnostics(@Nonnull PrintWriter w) throws IOException {
w.printf(" Last command sent=%s%n", new Date(lastCommandSentAt));
w.printf(" Last command received=%s%n", new Date(lastCommandReceivedAt));

// TODO: Update after the merge to 3.x branch, where the Hashtable is going to be replaced as a part of
// https://github.com/jenkinsci/remoting/pull/109
// TODO: Synchronize when Hashtable gets replaced by a modern collection.
w.printf(" Pending calls=%d%n", pendingCalls.size());
}

Expand Down Expand Up @@ -1413,6 +1416,7 @@ public void close(@CheckForNull Throwable diagnosis) throws IOException {
// termination is done by CloseCommand when we received it.
}

//TODO: ideally waitForProperty() methods should get rid of the notify-driven implementation
/**
* Gets the application specific property set by {@link #setProperty(Object, Object)}.
* These properties are also accessible from the remote channel via {@link #getRemoteProperty(Object)}.
Expand All @@ -1421,7 +1425,13 @@ public void close(@CheckForNull Throwable diagnosis) throws IOException {
* This mechanism can be used for one side to discover contextual objects created by the other JVM
* (as opposed to executing {@link Callable}, which cannot have any reference to the context
* of the remote {@link Channel}.
* @param key Key
* @return The property or {@code null} if there is no property for the specified key
*/
@Override
@SuppressFBWarnings(value = "UG_SYNC_SET_UNSYNC_GET",
justification = "setProperty() is synchronized in order to notify waitForProperty() methods" +
"No problem with this method since it is a ConcurrentHashMap")
public Object getProperty(Object key) {
return properties.get(key);
}
Expand Down Expand Up @@ -1459,10 +1469,13 @@ public <T> T waitForProperty(ChannelProperty<T> key) throws InterruptedException

/**
* Sets the property value on this side of the channel.
*
* @param key Property key
* @param value Value to set. {@code null} removes the existing entry without adding a new one.
* @return Old property value or {@code null} if it was not set
* @see #getProperty(Object)
*/
public synchronized Object setProperty(Object key, Object value) {
@CheckForNull
public synchronized Object setProperty(@Nonnull Object key, @CheckForNull Object value) {
Object old = value!=null ? properties.put(key, value) : properties.remove(key);
notifyAll();
return old;
Expand Down
3 changes: 2 additions & 1 deletion src/main/java/hudson/remoting/ChannelBuilder.java
Expand Up @@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Hashtable;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -316,7 +317,7 @@ public <T> ChannelBuilder withProperty(ChannelProperty<T> key, T value) {
* @since 2.47
*/
public Map<Object,Object> getProperties() {
return properties;
return Collections.unmodifiableMap(properties);
}

/**
Expand Down
4 changes: 0 additions & 4 deletions src/main/java/hudson/remoting/ChunkedOutputStream.java
Expand Up @@ -30,10 +30,6 @@ public ChunkedOutputStream(int frameSize, OutputStream base) {
this.base = base;
}

private int frameSize() {
return buf.length;
}

/**
* How many more bytes can our buffer take?
*/
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/hudson/remoting/ClassLoaderHolder.java
@@ -1,5 +1,6 @@
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.remoting.RemoteClassLoader.IClassLoader;
import org.jenkinsci.remoting.SerializableOnlyOverRemoting;

Expand Down Expand Up @@ -40,6 +41,8 @@ private void readObject(ObjectInputStream ois) throws IOException, ClassNotFound
classLoader = proxy==null ? null : getChannelForSerialization().importedClassLoaders.get(proxy);
}

@SuppressFBWarnings(value = "DMI_NONSERIALIZABLE_OBJECT_WRITTEN",
justification = "RemoteClassLoader.export() produces a serializable wrapper class")
private void writeObject(ObjectOutputStream oos) throws IOException {
if (classLoader==null)
oos.writeObject(null);
Expand Down
@@ -1,8 +1,11 @@
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.PrintWriter;
import java.io.StreamCorruptedException;
import java.io.StringWriter;
import java.util.Arrays;
import javax.annotation.Nonnull;

/**
* Signals a {@link StreamCorruptedException} with some additional diagnostic information.
Expand All @@ -11,10 +14,15 @@
*/
public class DiagnosedStreamCorruptionException extends StreamCorruptedException {
private final Exception diagnoseFailure;

@Nonnull
private final byte[] readBack;

@Nonnull
private final byte[] readAhead;

DiagnosedStreamCorruptionException(Exception cause, Exception diagnoseFailure, byte[] readBack, byte[] readAhead) {
DiagnosedStreamCorruptionException(Exception cause, Exception diagnoseFailure,
@Nonnull byte[] readBack, @Nonnull byte[] readAhead) {
initCause(cause);
this.diagnoseFailure = diagnoseFailure;
this.readBack = readBack;
Expand All @@ -25,12 +33,14 @@ public Exception getDiagnoseFailure() {
return diagnoseFailure;
}

@Nonnull
public byte[] getReadBack() {
return readBack;
return readBack.clone();
}


@Nonnull
public byte[] getReadAhead() {
return readAhead;
return readAhead.clone();
}

@Override
Expand Down
16 changes: 11 additions & 5 deletions src/main/java/hudson/remoting/ExportTable.java
Expand Up @@ -97,9 +97,12 @@ private final class Entry<T> {
*/
private int referenceCount;

//TODO: cleanup this mess?
/**
* This field can be set programmatically to track reference counting
* This field can be set programmatically to track reference counting.
* Please note that value unset is not thread-safe.
*/
@SuppressFBWarnings(value = "UWF_UNWRITTEN_FIELD", justification = "Old System script magic")
@CheckForNull
private ReferenceCountRecorder recorder;

Expand Down Expand Up @@ -460,13 +463,16 @@ private synchronized ExecutionException diagnoseInvalidObjectId(int id) {
Exception cause=null;

if (!unexportLog.isEmpty()) {
for (Entry e : unexportLog) {
for (Entry<?> e : unexportLog) {
if (e.id==id)
cause = new Exception("Object was recently deallocated\n"+Util.indent(e.dump()), e.releaseTrace);
}
if (cause==null)
cause = new Exception("Object appears to be deallocated at lease before "+
new Date(unexportLog.get(0).releaseTrace.timestamp));
if (cause==null) {
// If there is no cause available, create an artificial cause and use the last unexport entry as an estimated release time if possible
final ReleasedAt releasedAt = unexportLog.get(0).releaseTrace;
final Date releasedBefore = releasedAt != null ? new Date(releasedAt.timestamp) : new Date();
cause = new Exception("Object appears to be deallocated at lease before "+ releasedBefore);
}
}

return new ExecutionException("Invalid object ID "+id+" iota="+iota, cause);
Expand Down
13 changes: 9 additions & 4 deletions src/main/java/hudson/remoting/FileSystemJarCache.java
Expand Up @@ -6,6 +6,8 @@
import java.io.FileOutputStream;
import java.io.IOException;
import java.net.URL;
import java.nio.file.Files;
import java.nio.file.attribute.FileTime;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -69,9 +71,12 @@ protected URL lookInCache(Channel channel, long sum1, long sum2) throws IOExcept
File jar = map(sum1, sum2);
if (jar.exists()) {
LOGGER.log(Level.FINER, String.format("Jar file cache hit %16X%16X",sum1,sum2));
if (touch) jar.setLastModified(System.currentTimeMillis());
if (notified.add(new Checksum(sum1,sum2)))
if (touch) {
Files.setLastModifiedTime(jar.toPath(), FileTime.fromMillis(System.currentTimeMillis()));
}
if (notified.add(new Checksum(sum1,sum2))) {
getJarLoader(channel).notifyJarPresence(sum1,sum2);
}
return jar.toURI().toURL();
}
return null;
Expand All @@ -93,7 +98,7 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException
"Cached file checksum mismatch: %s%nExpected: %s%n Actual: %s",
target.getAbsolutePath(), expected, actual
));
target.delete();
Files.delete(target.toPath());
synchronized (checksumsByPath) {
checksumsByPath.remove(target.getCanonicalPath());
}
Expand Down Expand Up @@ -137,7 +142,7 @@ protected URL retrieve(Channel channel, long sum1, long sum2) throws IOException

return target.toURI().toURL();
} finally {
tmp.delete();
Files.deleteIfExists(tmp.toPath());
}
} catch (IOException e) {
throw (IOException)new IOException("Failed to write to "+target).initCause(e);
Expand Down
13 changes: 1 addition & 12 deletions src/main/java/hudson/remoting/InitializeJarCacheMain.java
Expand Up @@ -62,23 +62,12 @@ public static void main(String[] argv) throws Exception {
* our own from scratch.
*/
private static void copyFile(File src, File dest) throws Exception {
FileInputStream input = null;
FileOutputStream output = null;
try {
input = new FileInputStream(src);
output = new FileOutputStream(dest);
try (FileInputStream input = new FileInputStream(src); FileOutputStream output = new FileOutputStream(dest)) {
byte[] buf = new byte[1024 * 1024];
int bytesRead;
while ((bytesRead = input.read(buf)) > 0) {
output.write(buf, 0, bytesRead);
}
} finally {
if (input != null) {
input.close();
}
if (output != null) {
output.close();
}
}
}
}
3 changes: 3 additions & 0 deletions src/main/java/hudson/remoting/PipeWindow.java
Expand Up @@ -23,6 +23,7 @@
*/
package hudson.remoting;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.io.IOException;
import java.io.OutputStream;
import java.util.logging.Logger;
Expand Down Expand Up @@ -155,6 +156,8 @@ public int hashCode() {
}
}

//TODO: Consider rework and cleanup of the fields
@SuppressFBWarnings(value = "URF_UNREAD_FIELD", justification = "Legacy implementation")
static class Real extends PipeWindow {
private final int initial;
private int available;
Expand Down

0 comments on commit 49c67ee

Please sign in to comment.