I did another pass.
If there was a way for jsr166 maintainers to see these warnings, we could
more easily fix ourselves. But there is no such thing as a "preview javac
warning", eh?
---
The comments for Condition declarations are still not consistent - every
one is known to be Serializable, but they cannot be declared Condition &
Serializable because Java does not have first-class support for
intersection types. So just make all of these:
@SuppressWarnings("serial") // Not statically typed as Serializable
---
All of the suppressions in ForkJoinTask below should be
// Conditionally serializable
because serializability depends on user-supplied action, and T could be any
type.
--- old/src/java.base/share/classes/java/util/concurrent/ForkJoinTask.java
2019-10-15 23:58:37.836743608 -0700
+++ new/src/java.base/share/classes/java/util/concurrent/ForkJoinTask.java
2019-10-15 23:58:37.624743608 -0700
@@ -1374,7 +1374,9 @@
*/
static final class AdaptedRunnable<T> extends ForkJoinTask<T>
implements RunnableFuture<T> {
+ @SuppressWarnings("serial") // Not statically typed as Serializable
final Runnable runnable;
+ @SuppressWarnings("serial") // Not statically typed as Serializable
T result;
AdaptedRunnable(Runnable runnable, T result) {
if (runnable == null) throw new NullPointerException();
@@ -1396,6 +1398,7 @@
*/
static final class AdaptedRunnableAction extends ForkJoinTask<Void>
implements RunnableFuture<Void> {
+ @SuppressWarnings("serial") // Not statically typed as Serializable
final Runnable runnable;
AdaptedRunnableAction(Runnable runnable) {
if (runnable == null) throw new NullPointerException();
@@ -1415,6 +1418,7 @@
* Adapter for Runnables in which failure forces worker exception.
*/
static final class RunnableExecuteAction extends ForkJoinTask<Void> {
+ @SuppressWarnings("serial") // Not statically typed as Serializable
final Runnable runnable;
RunnableExecuteAction(Runnable runnable) {
if (runnable == null) throw new NullPointerException();
@@ -1434,7 +1438,9 @@
*/
static final class AdaptedCallable<T> extends ForkJoinTask<T>
implements RunnableFuture<T> {
+ @SuppressWarnings("serial") // Not statically typed as Serializable
final Callable<? extends T> callable;
+ @SuppressWarnings("serial") // Not statically typed as Serializable
T result;
AdaptedCallable(Callable<? extends T> callable) {
if (callable == null) throw new NullPointerException();
---
For Thread objects, I would say
// unlikely to be serializable
---
old/src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java
2019-10-15 23:58:40.460743608 -0700
+++
new/src/java.base/share/classes/java/util/concurrent/ThreadPoolExecutor.java
2019-10-15 23:58:40.256743608 -0700
@@ -604,8 +604,10 @@
private static final long serialVersionUID = 6138294804551838833L;
/** Thread this worker is running in. Null if factory fails. */
+ @SuppressWarnings("serial") // Not statically typed as Serializable
final Thread thread;
/** Initial task to run. Possibly null. */
On Wed, Oct 16, 2019 at 12:03 AM Joe Darcy <joe.da...@oracle.com> wrote:
> Hi Martin,
> On 10/15/2019 8:02 AM, Martin Buchholz wrote:
>
>
>
> On Mon, Oct 14, 2019 at 10:48 PM Joe Darcy <joe.da...@oracle.com> wrote:
>
>> Hi Martin,
>> On 10/14/2019 10:28 PM, Martin Buchholz wrote:
>>
>> Hi Joe,
>>
>> I'm surprised there were so few changes.
>>
>>
>> The changes in question allow the java.util.concurrent package to pass
>> cleanly through the current version of my checks. Perhaps a strong version
>> of the checks in the future will flag more issues of possible concern ;-)
>>
>>
>>
>> Why are some of the Condition's marked // Conditionally serializable
>>
>> Summarizing the discussion leading up to the changes for JDK-8231913,
>> "Conditionally
>> serializable" describes collections that are serializable iff their
>> contents are serializable. The changes made for
>>
>> 8231202: Suppress warnings on non-serializable non-transient instance
>> fields in serializable classes
>> http://hg.openjdk.java.net/jdk/jdk/rev/e036ee8bae56
>>
>> use this terminology in the comments added for many collection types,
>> including Vector, TreeMap, and multiple implementation classes in
>> java.util.Collections. I tried to follow the same pattern in
>> java.util.concurrent where appropriate.
>>
> The Condition field in ArrayBlockingQueue plays exactly the same role as
> in e.g. LinkedBlockingQueue, so should have exactly the same treatment.
>
>
> Changed the comments on the Condition-typed fields to
>
> // Classes implementing Condition may be serializable.
>
> Revised webrev at:
>
> http://cr.openjdk.java.net/~darcy/8232230.1/
>
> ---
> All Condition implementations in OpenJDK are Serializable.
> In fact, we could promote all of those private Condition fields to
> ConditionObject, making their Serializability clear, and then elide the
> need for suppression.
>
> private final ConditionObject notEmpty = (ConditionObject)
> takeLock.newCondition();
>
> (Why won't Java let me declare those fields private final Condition &
> Serializable ?)
>
> but now we're drifting into actual user-meaningful development... We
> promise that ReentrantLock is serializable, but we don't seem to have any
> such promise for Conditions returned from ReentrantLock.newCondition(), and
> we probably should. Users are implicitly relying on the Serializability of
> Condition in the same way we are.
>
>
> I've filed JDK-8232359: "Review type of fields in Serializable classes of
> java.util.concurrent" to track this potential future cleanup.
>
>
>
> ---
>
> Joe, go ahead and commit this to openjdk head when review is done. We'll
> incorporate.
>
>
> Acknowledged; thanks,
>
> -Joe
>
