Skip to content

Commit

Permalink
Added serialVersionUID to all Callables
Browse files Browse the repository at this point in the history
  • Loading branch information
@kutzi
kutzi committed Jan 20, 2013
1 parent 19b3168 commit b807845
Showing 1 changed file with 29 additions and 0 deletions.
29 changes: 29 additions & 0 deletions core/src/main/java/hudson/FilePath.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,7 @@ private void unzip(File dir, File zipFile) throws IOException {
*/
public FilePath absolutize() throws IOException, InterruptedException {
return new FilePath(channel,act(new FileCallable<String>() {
private static final long serialVersionUID = 1L;
public String invoke(File f, VirtualChannel channel) throws IOException {
return f.getAbsolutePath();
}
Expand All @@ -560,6 +561,7 @@ public String invoke(File f, VirtualChannel channel) throws IOException {
*/
public void symlinkTo(final String target, final TaskListener listener) throws IOException, InterruptedException {
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
Util.createSymlink(f.getParentFile(),target,f.getName(),listener);
return null;
Expand All @@ -576,6 +578,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException, Interrupt
*/
public String readLink() throws IOException, InterruptedException {
return act(new FileCallable<String>() {
private static final long serialVersionUID = 1L;
public String invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
return Util.resolveSymlink(f);
}
Expand Down Expand Up @@ -884,6 +887,7 @@ public static abstract class FileCallableWrapperFactory implements ExtensionPoin
* @since 1.482
*/
public static abstract class AbstractInterceptorCallableWrapper<T> implements DelegatingCallable<T, IOException> {
private static final long serialVersionUID = 1L;

private final DelegatingCallable<T, IOException> callable;

Expand Down Expand Up @@ -960,6 +964,7 @@ public <V,E extends Throwable> V act(Callable<V,E> callable) throws IOException,
*/
public URI toURI() throws IOException, InterruptedException {
return act(new FileCallable<URI>() {
private static final long serialVersionUID = 1L;
public URI invoke(File f, VirtualChannel channel) {
return f.toURI();
}
Expand All @@ -971,6 +976,7 @@ public URI invoke(File f, VirtualChannel channel) {
*/
public void mkdirs() throws IOException, InterruptedException {
if(!act(new FileCallable<Boolean>() {
private static final long serialVersionUID = 1L;
public Boolean invoke(File f, VirtualChannel channel) throws IOException, InterruptedException {
if(f.mkdirs() || f.exists())
return true; // OK
Expand All @@ -989,6 +995,7 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException, Interr
*/
public void deleteRecursive() throws IOException, InterruptedException {
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
Util.deleteRecursive(f);
return null;
Expand All @@ -1001,6 +1008,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException {
*/
public void deleteContents() throws IOException, InterruptedException {
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
Util.deleteContentsRecursive(f);
return null;
Expand Down Expand Up @@ -1094,6 +1102,7 @@ public FilePath getParent() {
public FilePath createTempFile(final String prefix, final String suffix) throws IOException, InterruptedException {
try {
return new FilePath(this,act(new FileCallable<String>() {
private static final long serialVersionUID = 1L;
public String invoke(File dir, VirtualChannel channel) throws IOException {
File f = File.createTempFile(prefix, suffix, dir);
return f.getName();
Expand Down Expand Up @@ -1149,6 +1158,7 @@ public FilePath createTextTempFile(final String prefix, final String suffix, fin
public FilePath createTextTempFile(final String prefix, final String suffix, final String contents, final boolean inThisDirectory) throws IOException, InterruptedException {
try {
return new FilePath(channel,act(new FileCallable<String>() {
private static final long serialVersionUID = 1L;
public String invoke(File dir, VirtualChannel channel) throws IOException {
if(!inThisDirectory)
dir = new File(System.getProperty("java.io.tmpdir"));
Expand Down Expand Up @@ -1191,6 +1201,7 @@ public String invoke(File dir, VirtualChannel channel) throws IOException {
public FilePath createTempDir(final String prefix, final String suffix) throws IOException, InterruptedException {
try {
return new FilePath(this,act(new FileCallable<String>() {
private static final long serialVersionUID = 1L;
public String invoke(File dir, VirtualChannel channel) throws IOException {
File f = File.createTempFile(prefix, suffix, dir);
f.delete();
Expand All @@ -1210,6 +1221,7 @@ public String invoke(File dir, VirtualChannel channel) throws IOException {
*/
public boolean delete() throws IOException, InterruptedException {
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
Util.deleteFile(f);
return null;
Expand All @@ -1223,6 +1235,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException {
*/
public boolean exists() throws IOException, InterruptedException {
return act(new FileCallable<Boolean>() {
private static final long serialVersionUID = 1L;
public Boolean invoke(File f, VirtualChannel channel) throws IOException {
return f.exists();
}
Expand All @@ -1238,6 +1251,7 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException {
*/
public long lastModified() throws IOException, InterruptedException {
return act(new FileCallable<Long>() {
private static final long serialVersionUID = 1L;
public Long invoke(File f, VirtualChannel channel) throws IOException {
return f.lastModified();
}
Expand Down Expand Up @@ -1289,6 +1303,7 @@ public String invoke(File f, VirtualChannel channel) throws IOException {
*/
public boolean isDirectory() throws IOException, InterruptedException {
return act(new FileCallable<Boolean>() {
private static final long serialVersionUID = 1L;
public Boolean invoke(File f, VirtualChannel channel) throws IOException {
return f.isDirectory();
}
Expand All @@ -1302,6 +1317,7 @@ public Boolean invoke(File f, VirtualChannel channel) throws IOException {
*/
public long length() throws IOException, InterruptedException {
return act(new FileCallable<Long>() {
private static final long serialVersionUID = 1L;
public Long invoke(File f, VirtualChannel channel) throws IOException {
return f.length();
}
Expand All @@ -1326,6 +1342,7 @@ public Long invoke(File f, VirtualChannel channel) throws IOException {
public void chmod(final int mask) throws IOException, InterruptedException {
if(!isUnix() || mask==-1) return;
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
_chmod(f, mask);

Expand Down Expand Up @@ -1376,6 +1393,7 @@ private static void _chmodAnt(File f, int mask) {
public int mode() throws IOException, InterruptedException, PosixException {
if(!isUnix()) return -1;
return act(new FileCallable<Integer>() {
private static final long serialVersionUID = 1L;
public Integer invoke(File f, VirtualChannel channel) throws IOException {
return IOUtils.mode(f);
}
Expand Down Expand Up @@ -1422,6 +1440,7 @@ public List<FilePath> list(final FileFilter filter) throws IOException, Interrup
throw new IllegalArgumentException("Non-serializable filter of " + filter.getClass());
}
return act(new FileCallable<List<FilePath>>() {
private static final long serialVersionUID = 1L;
public List<FilePath> invoke(File f, VirtualChannel channel) throws IOException {
File[] children = f.listFiles(filter);
if(children ==null) return null;
Expand Down Expand Up @@ -1474,6 +1493,7 @@ public FilePath[] list(final String includes, final String excludes) throws IOEx
*/
public FilePath[] list(final String includes, final String excludes, final boolean defaultExcludes) throws IOException, InterruptedException {
return act(new FileCallable<FilePath[]>() {
private static final long serialVersionUID = 1L;
public FilePath[] invoke(File f, VirtualChannel channel) throws IOException {
String[] files = glob(f, includes, excludes, defaultExcludes);

Expand Down Expand Up @@ -1511,6 +1531,7 @@ public InputStream read() throws IOException {

final Pipe p = Pipe.createRemoteToLocal();
channel.callAsync(new Callable<Void,IOException>() {
private static final long serialVersionUID = 1L;
public Void call() throws IOException {
FileInputStream fis=null;
try {
Expand Down Expand Up @@ -1562,6 +1583,7 @@ public OutputStream write() throws IOException, InterruptedException {
}

return channel.call(new Callable<OutputStream,IOException>() {
private static final long serialVersionUID = 1L;
public OutputStream call() throws IOException {
File f = new File(remote).getAbsoluteFile();
f.getParentFile().mkdirs();
Expand All @@ -1580,6 +1602,7 @@ public OutputStream call() throws IOException {
*/
public void write(final String content, final String encoding) throws IOException, InterruptedException {
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
f.getParentFile().mkdirs();
FileOutputStream fos = new FileOutputStream(f);
Expand All @@ -1599,6 +1622,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException {
*/
public String digest() throws IOException, InterruptedException {
return act(new FileCallable<String>() {
private static final long serialVersionUID = 1L;
public String invoke(File f, VirtualChannel channel) throws IOException {
return Util.getDigestOf(new FileInputStream(f));
}
Expand All @@ -1614,6 +1638,7 @@ public void renameTo(final FilePath target) throws IOException, InterruptedExcep
throw new IOException("renameTo target must be on the same host");
}
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
f.renameTo(new File(target.remote));
return null;
Expand All @@ -1631,6 +1656,7 @@ public void moveAllChildrenTo(final FilePath target) throws IOException, Interru
throw new IOException("pullUpTo target must be on the same host");
}
act(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
File t = new File(target.getRemote());

Expand Down Expand Up @@ -1783,6 +1809,7 @@ public int copyRecursiveTo(final String fileMask, final String excludes, final F
if(this.channel==target.channel) {
// local to local copy.
return act(new FileCallable<Integer>() {
private static final long serialVersionUID = 1L;
public Integer invoke(File base, VirtualChannel channel) throws IOException {
if(!base.exists()) return 0;
assert target.channel==null;
Expand Down Expand Up @@ -1825,6 +1852,7 @@ public int getNumCopied() {
final Pipe pipe = Pipe.createLocalToRemote();

Future<Void> future = target.actAsync(new FileCallable<Void>() {
private static final long serialVersionUID = 1L;
public Void invoke(File f, VirtualChannel channel) throws IOException {
try {
readFromTar(remote+'/'+fileMask, f,TarCompression.GZIP.extract(pipe.getIn()));
Expand All @@ -1846,6 +1874,7 @@ public Void invoke(File f, VirtualChannel channel) throws IOException {
final Pipe pipe = Pipe.createRemoteToLocal();

Future<Integer> future = actAsync(new FileCallable<Integer>() {
private static final long serialVersionUID = 1L;
public Integer invoke(File f, VirtualChannel channel) throws IOException {
try {
return writeToTar(f,fileMask,excludes,TarCompression.GZIP.compress(pipe.getOut()));
Expand Down

8 comments on commit b807845

@jglick
Copy link
Member

@jglick jglick commented on b807845 Jan 22, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary and possibly undesirable—adding a serialVersionUID makes a promise, which you are probably not going to keep, that all future versions of this class will be compatible in their serial form (unless the modifier changes the SVUID too). Omitting the UID just means that the remoting channel will throw an exception when the two sides do not agree on the content of the class, which is much better than the communication silently proceeding but some fields being null without explanation.

The real threat is that the serial form of anonymous inner classes is unstable not because of changes in variables in its closure (thus effective fields), but because lexical insertion of another such class above it could change it from e.g. FilePath$13 to FilePath$14. (Which would be especially catastrophic if one side has a FilePath$13 with SVUID=1 and the other side has a FilePath$13 also with SVUID=1 which are in fact totally unrelated callables!) Therefore it is safer to refactor all anonymous FileCallable instances into static nested classes.

See also proposed NetBeans plugin hint and examples such as 8a1b069 or 9b0e1fb.

@kutzi
Copy link
Member Author

@kutzi kutzi commented on b807845 Jan 23, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jesse, I generally agree about what you say about serialVersionUIDs. But IMO we usually don't have the problem here that 2 different versions of a class are loaded in different VMs, but that the same version is loaded in 2 VMs of different types and the VMs disagree on the computed versionUID.
I had already such bugs fixed in the past where the master was running on Oracle VM and a slave on J9 VM and they disagreed on the UID.
Setting the serialVersionUID fixed that bug.

@kutzi
Copy link
Member Author

@kutzi kutzi commented on b807845 Jan 23, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting the callables to static classes is a good idea in any way. It's just that they were too many...

@jglick
Copy link
Member

@jglick jglick commented on b807845 Jan 23, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a published algorithm for computing the SVUID from a given class, so if J9 gets it wrong then that is a serious bug in that JVM.

(It is fine for alternate compilers to produce subtly different classes from the same source, which could then have different default SVUIDs; but that would not be the issue here since the Jenkins master sends the same byte[] bytecode to the slave that it is loading itself—typically whatever the core or plugin developer compiled when performing a Maven release.)

Regarding refactoring to static classes, this should be a one-step operation in any modern Java IDE.

@kutzi
Copy link
Member Author

@kutzi kutzi commented on b807845 Jan 26, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, be it an error in J9 or not (apparently the algorithm for the UID is not as precise as it looks like on the 1st look - see e.g http://c2.com/cgi/wiki?AlwaysDeclareSerialVersionUid), fact is that we have to cope with it. And as adding a serialversionUID isn't much effort, I see no reason why we shouldn't.

@jglick
Copy link
Member

@jglick jglick commented on b807845 Jan 28, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just that, as I say, it can be actively harmful if you do add new fields later (or make other important structural changes) and fail to change the SVUID. Declaring an explicit SVUID is necessary if you plan to deserialize objects created by a different version of the code, and are explicitly testing and maintaining serial form compatibility; but this is not the case for Jenkins remoting, where master and slave ought to always be running identical bytecode.

@jglick
Copy link
Member

@jglick jglick commented on b807845 Feb 1, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW this seems to have caused an error for a dev build user though I am not yet sure why:

hudson.util.IOException2: remote file operation failed: …/hudson/workspace/… at hudson.remoting.Channel@…
        at hudson.FilePath.act(FilePath.java:847) 
        at hudson.FilePath.act(FilePath.java:824) 
        at hudson.FilePath.mkdirs(FilePath.java:962) 
        at hudson.model.AbstractProject.checkout(AbstractProject.java:1323) 
        at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:676) 
        at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:88) 
        at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:581) 
        at hudson.model.Run.execute(Run.java:1516) 
        at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:46) 
        at hudson.model.ResourceController.execute(ResourceController.java:88) 
        at hudson.model.Executor.run(Executor.java:236) 
Caused by: java.io.InvalidClassException: hudson.FilePath$FileCallableWrapper; local class incompatible: stream classdesc serialVersionUID = 1, local class serialVersionUID = 1131869274958374009
        …

@jglick
Copy link
Member

@jglick jglick commented on b807845 Feb 1, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, that was from an older build; perhaps this commit corrected it.

Please sign in to comment.