Thread safety, "array of singletons".

[Daniel Pitts]

I have a "Card" class which has objects that represent cards out of a
standard poker deck. I statically initialize the array.

My basic question is: Are there circumstances in which the following
code will not produce "==" comparable Card objects? These objects may
be passed through RMI or otherwise serialized.

<sscce>
import java.io.InvalidObjectException;
import java.ibjectStreamException;

public final class Card {
static final long serialVersionUID = 1L;

public enum Suit {
DIAMOND,
CLUB,
HEART,
SPADE;
}

public static final int SUITS = Suit.values().length;
public static final int CARDS_PER_SUIT = 13;
public static final int NUM_CARDS = SUITS * CARDS_PER_SUIT;
private static final Card[] cards = new Card[NUM_CARDS];
private static final String[] numbers = new String[NUM_CARDS];

public static final int JACK_ORDINAL = 10;
private static final int QUEEN_ORDINAL = JACK_ORDINAL + 1;
private static final int KING_ORDINAL = QUEEN_ORDINAL + 1;

static {
numbers[0] = "A";
for (int i = 1; i < JACK_ORDINAL; ++i) {
numbers = String.valueOf(i + 1);
}
numbers[JACK_ORDINAL] = "J";
numbers[QUEEN_ORDINAL] = "Q";
numbers[KING_ORDINAL] = "K";
for (int i = 0; i < NUM_CARDS; ++i) {
cards = new Card(i);
}
}

public final transient Suit suit;
public final transient String number;
public final transient int value;
public final int ordinal;

private Card(int ordinal) {
this.ordinal = ordinal;
final int suitOrdinal = ordinal / CARDS_PER_SUIT;
final int cardOrdinal = ordinal % CARDS_PER_SUIT;
value = cardOrdinal == 0 ? KING_ORDINAL : cardOrdinal - 1;
suit = Suit.values()[suitOrdinal];
number = numbers[cardOrdinal];
}

protected final Object clone() throws CloneNotSupportedException {
throw new CloneNotSupportedException();
}

protected final Object readResolve() throws ObjectStreamException {
if (ordinal < 0 || NUM_CARDS <= ordinal) {
throw new InvalidObjectException("Not a valid card.");
}
return forOrdinal(ordinal);
}

public static Card forOrdinal(int ordinal) {
return cards[ordinal];
}
}
</sscce>

 

[Tom Hawtin]

I have a "Card" class which has objects that represent cards out of a
standard poker deck. I statically initialize the array.

My basic question is: Are there circumstances in which the following
code will not produce "==" comparable Card objects? These objects may
be passed through RMI or otherwise serialized.

I believe, malicious code could grab a deserialised instance before
readResolve, but other than that I think you have covered all the bases.

import java.io.InvalidObjectException;
import java.ibjectStreamException;

public final class Card {
static final long serialVersionUID = 1L;

You mean:

public final class Card implements java.io.Serializable {
private static final long serialVersionUID = 1L;

public final transient Suit suit;
public final transient String number;
public final transient int value;

I would calculate these on demand.

protected final Object clone() throws CloneNotSupportedException {
throw new CloneNotSupportedException();
}

The class is final, so why bother?

protected final Object readResolve() throws ObjectStreamException {

Why protected?

Tom Hawtin

 

[Daniel Pitts]

I believe, malicious code could grab a deserialised instance before
readResolve, but other than that I think you have covered all the bases.

I'm not worried about malicious code in this place. the value only
"really" matters on a server side, which shouldn't load or execute
untrusted code at all. But out of curiosity, how WOULD malicious code
obtain a pre-readResolved reference? It doesn't matter in this case,
but if its possible, I should learn about it

You mean:

public final class Card implements java.io.Serializable {
private static final long serialVersionUID = 1L; Yes I did.


I would calculate these on demand.

Perhaps, but I'm avoiding getters here, and there seems little reason
to not pre-calculate these values.

The class is final, so why bother?

Why bother? Just in case Defensive programmming...
Having it declared this way lets future developers know it should not
be changed. This is actually noted in a comment that I stripped out.
It is also stolen from java.lang.Enum;

Why protected?

Well, my IDE complains about unused private methods, and public seems
a bad idea, as well as package-protected.

Anyway, other than malicious readResolve interceptors, this looks like
it would be safe and correct code?

Thanks,
Daniel.

 

[Chris Uppal]

I would calculate these on demand.

Why ? If you make them non-final then they aren't threadsafe.

-- chris

 

[Piotr Kobzda]

Well, my IDE complains about unused private methods, and public seems
a bad idea, as well as package-protected.

Using protected is equally bed idea as using package-protected... You
should use @SuppressWarnings("unused") to calm down the IDE.


piotr

 

[Tom Hawtin]

Why ? If you make them non-final then they aren't threadsafe.

I meant, I would calculate the values every time. I wouldn't have them
as fields at all. So, for instance, there would be a method getNumber:

public String getNumber() {
return numbers[ordinal % CARDS_PER_SUIT];
}

Certainly lazy initialisation is usually not a winner. However, in this
case it would not actually remove thread-safety for the same reason
String.hash works.

Tom Hawtin

 

[Chris Uppal]

I meant, I would calculate the values every time. I wouldn't have them
as fields at all.

Ah, I see. A valid design choice...

Certainly lazy initialisation is usually not a winner. However, in this
case it would not actually remove thread-safety for the same reason
String.hash works.

Point taken, and agreed.

-- chris

 

[Hendrik Maryns]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Tom Hawtin schreef:

Why ? If you make them non-final then they aren't threadsafe.

I meant, I would calculate the values every time. I wouldn't have them
as fields at all. So, for instance, there would be a method getNumber:

public String getNumber() {
return numbers[ordinal % CARDS_PER_SUIT];
}

Certainly lazy initialisation is usually not a winner.

Can you elaborate on this a bit? Is it only with respect to
multithreading, or in general?

H.
- --
Hendrik Maryns
http://tcl.sfs.uni-tuebingen.de/~hendrik/
==================
http://aouw.org
Ask smart questions, get good answers:
http://www.catb.org/~esr/faqs/smart-questions.html
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)

iD8DBQFGCOmge+7xMGD3itQRAgB2AJ460w04hTazd0YpOmXvpXiUgtsWbACggoLg
wBdcvE5kG8LXaq9TSyf0sZM=
=sGfv
-----END PGP SIGNATURE-----

 

[Tom Hawtin]

Tom Hawtin schreef:

Can you elaborate on this a bit? Is it only with respect to
multithreading, or in general?

In general.

It can be a win, but the circumstances are relatively unusual. The
computation needs to be significant in terms of CPU cycles or memory,
and also it needs to be unnecessary to evaluate more often than not.

Lazy initialisation can lose in a number of ways:

* As you say, overhead in dealing with multiple threads.
* The code becomes more complicated and therefore more difficult to
understand and very much more likely to be buggy.
* Checking for initialisation tends to make the code slower. It's not
just the extra check, but the amount of code potentially runnable within
performance critical sections is also massively increased. Big sections
are code are very difficult to optimise, particularly in a system that
compiles at runtime.
* If the initialisation is done always, or almost always, then you are
not gaining anything anyway.

It's one of those techniques that sounds really cool, but actually is
rarely useful. The result is that it tends to get used inappropriately.

Tom Hawtin

 

[Daniel Pitts]

Ah, I see. A valid design choice...


Point taken, and agreed.

-- chris

It may not remove thread safety, but I think it actually complicates
the design. I think my original class is simplified by having a few
precalculated values and a clear process which calculates them.