Take advantage of Java 5 language features #389

basil · 2020-07-02T18:18:30Z

Downstream of #388. This PR updates the code base to take full advantage of Java 5 language features; namely, enhanced for and generics. There are two commits in this change (on top of the commit from #388). I suggest reviewing each commit separately. The first commit should be fairly self-explanatory and replaces any for loops with Java 5 enhanced for. The second commit adds generics to any types that were missing them and also tightens the bounds of these generics. I will explain these changes in a review.

This change may appear risky at first glance but it is actually completely safe due to the fact that no runtime code is affected. Yes, type parameters have been changed, but they only affect compilation and not runtime due to type erasure. There are only two places where I introduced casts in non-test code, but those casts are safe because they only cast from one set of type parameters to another set of type parameters (not from one type of object to another type of object). In other words, there is no runtime impact whatsoever.

basil · 2020-07-02T18:22:54Z

src/main/java/hudson/remoting/ExportTable.java

@@ -360,7 +360,7 @@ boolean isRecording() {
    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);
+        Entry<T> e = (Entry<T>) reverse.get(t);


This cast is safe, as can be seen by reading the surrounding code that places entries into the map. The keys of the map are of type Object and the values are of type Entry. The only time that we ever add something to the map, the Object is parameterized on T and the Entry is also parameterized on the same T.

basil · 2020-07-02T18:28:33Z

src/main/java/hudson/remoting/Request.java

@@ -79,7 +79,7 @@
     */
    private int lastIoId;

-    private volatile Response<RSP,EXC> response;
+    private volatile Response<RSP,? extends Throwable> response;


The original type parameter was wrong. Fortunately, this never resulted in any actual bugs due to type erasure. The reasoning is as follows. The EXC type parameter in the Request class represents the bound for the exception types that subclasses of Request may produce. The Response class may include exceptions of these types, but it may also include an exception of type RequestAbortedException (see Request#abort(IOException)). Recall that this field is in the Request class, so EXC represents the bound on the request's exception types, not the response's exception types. To keep supporting Request#abort(IOException), the bound here must be able to include RequestAbortedException. So the bound for EXC is not large enough, so we must include all instances of Throwable. This also prevents us from having to make a risky change to the Runnable in Request#execute.

basil · 2020-07-02T18:29:00Z

src/main/java/hudson/remoting/Request.java

@@ -192,10 +192,10 @@ final RSP call(Channel channel) throws EXC, InterruptedException, IOException {
                    // ignore the I/O error
                }

-                Object exc = response.exception;
+                Throwable exc = response.exception;


response.exception was always of type Throwable, so this was just unnecessary.

basil · 2020-07-02T18:29:49Z

src/main/java/hudson/remoting/Request.java

@@ -341,7 +341,7 @@ public RSP get(long timeout, TimeUnit unit) throws InterruptedException, Executi
     * Aborts the processing. The calling thread will receive an exception.
     */
    /*package*/ void abort(IOException e) {
-        onCompleted(new Response(this, id, 0, new RequestAbortedException(e)));
+        onCompleted(new Response<>(this, id, 0, new RequestAbortedException(e)));


This change would not have worked if we had not expanded the bound above to be able to include RequestAbortedException.

basil · 2020-07-02T18:32:15Z

src/main/java/hudson/remoting/Response.java

@@ -36,7 +37,7 @@
 * @see Request
 * @since 3.17
 */
-public final class Response<RSP,EXC extends Throwable> extends Command {
+public final class Response<RSP extends Serializable,EXC extends Throwable> extends Command {


Instances of Response are always created from instances of Request, whose RSP type parameter is bounded by Serializable. So there is no reason not to similarly bound the type parameter of Response. Doing so makes the relationship between the two more clear. Note that the bound of the EXC type parameter on the Response class may be different (broader) than that of the Request class due to the fact that a response may include a RequestAbortedException as described above.

basil · 2020-07-02T18:33:02Z

src/main/java/hudson/remoting/Response.java

@@ -59,17 +60,17 @@
    @SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Only supposed to be defined on one side.")
    private transient long totalTime;
    @SuppressFBWarnings(value="SE_TRANSIENT_FIELD_NOT_RESTORED", justification="Bound after deserialization, in execute.")
-    private transient Request<?, ?> request;
+    private transient Request<RSP, ? extends Throwable> request;


As in Request, we cannot use EXC here because the bound for the Response exception must be larger than those of the Request exception.

basil · 2020-07-02T18:35:00Z

src/main/java/hudson/remoting/Response.java

@@ -82,7 +83,7 @@
     */
    @Override
    void execute(Channel channel) {
-        Request req = channel.pendingCalls.get(id);
+        Request<RSP, ? extends Throwable> req = (Request<RSP, ? extends Throwable>) channel.pendingCalls.get(id);


This cast is safe because the RSP type parameter shares the same bounds for a Request and its Response.

basil · 2020-07-02T18:45:35Z

My CI run "failed" in the Windows branch, but this failure is spurious because the mvn(1) command actually succeeded. The overall branch was erroneously marked as failed due to a Remoting exception. 😉

jeffret-b · 2020-07-02T18:52:16Z

Let's try running the CI again.

jeffret-b · 2020-07-02T19:07:51Z

Build now passes. I'll take a look at the proposal when I get the chance.

jeffret-b

Nice improvements. This will help the maintainability and readability.

Thanks for the submission.

basil · 2020-10-14T22:36:43Z

Thanks for the submission.

Thank you for the review!

basil added 3 commits July 1, 2020 15:45

Add missing @OverRide annotations

6d52119

Replace 'for' loop with enhanced 'for'

93f4436

Add missing type parameters

eb17f2f

basil commented Jul 2, 2020

View reviewed changes

jeffret-b closed this Jul 2, 2020

jeffret-b reopened this Jul 2, 2020

jeffret-b approved these changes Oct 14, 2020

View reviewed changes

jeffret-b added chore For changelog: A maintenance chore with no functional changes ready-to-merge labels Oct 14, 2020

jeffret-b merged commit 68c21f7 into jenkinsci:master Oct 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Take advantage of Java 5 language features #389

Take advantage of Java 5 language features #389

basil commented Jul 2, 2020 •

edited

Loading

basil Jul 2, 2020

basil Jul 2, 2020

basil Jul 2, 2020

basil Jul 2, 2020

basil Jul 2, 2020

basil Jul 2, 2020

basil Jul 2, 2020

basil commented Jul 2, 2020

jeffret-b commented Jul 2, 2020

jeffret-b commented Jul 2, 2020

jeffret-b left a comment

basil commented Oct 14, 2020

Take advantage of Java 5 language features #389

Take advantage of Java 5 language features #389

Conversation

basil commented Jul 2, 2020 • edited Loading

basil Jul 2, 2020

Choose a reason for hiding this comment

basil Jul 2, 2020

Choose a reason for hiding this comment

basil Jul 2, 2020

Choose a reason for hiding this comment

basil Jul 2, 2020

Choose a reason for hiding this comment

basil Jul 2, 2020

Choose a reason for hiding this comment

basil Jul 2, 2020

Choose a reason for hiding this comment

basil Jul 2, 2020

Choose a reason for hiding this comment

basil commented Jul 2, 2020

jeffret-b commented Jul 2, 2020

jeffret-b commented Jul 2, 2020

jeffret-b left a comment

Choose a reason for hiding this comment

basil commented Oct 14, 2020

basil commented Jul 2, 2020 •

edited

Loading