Pourquoi BufferedInputStream copie-t-il un champ dans une variable locale plutôt que d'utiliser le champ directement

107

Quand je lis le code source à partir de java.io.BufferedInputStream.getInIfOpen(), je ne comprends pas pourquoi il a écrit un code comme celui-ci:

/**
 * Check to make sure that underlying input stream has not been
 * nulled out due to close; if not return it;
 */
private InputStream getInIfOpen() throws IOException {
    InputStream input = in;
    if (input == null)
        throw new IOException("Stream closed");
    return input;
}

Pourquoi utilise-t-il l'alias au lieu d'utiliser la variable de champ indirectement comme ci-dessous:

/**
 * Check to make sure that underlying input stream has not been
 * nulled out due to close; if not return it;
 */
private InputStream getInIfOpen() throws IOException {
    if (in == null)
        throw new IOException("Stream closed");
    return in;
}

Quelqu'un peut-il donner une explication raisonnable?

Saint
la source
Dans Eclipse, vous ne pouvez pas suspendre un débogueur sur une ifinstruction. Peut- être une raison pour cette variable alias. Je voulais juste jeter ça là-bas. Je spécule, bien sûr.
Debosmit Ray
@DebosmitRay: Vous ne pouvez vraiment pas faire une pause sur la ifdéclaration?
rkosegi
@rkosegi Sur ma version d'Eclipse, le problème est similaire à celui-ci . Ce n'est peut-être pas un événement très courant. Et de toute façon, je ne le pensais pas sur une note légère (clairement une mauvaise blague). :)
Debosmit Ray

Réponses:

119

Si vous regardez ce code hors de son contexte, il n'y a pas de bonne explication pour cet "alias". Il s'agit simplement d'un code redondant ou d'un style de code médiocre.

Mais le contexte est qu'il BufferedInputStreams'agit d'une classe qui peut être sous-classée et qu'elle doit fonctionner dans un contexte multi-thread.

L'indice est qu'il inest déclaré dans FilterInputStreamis protected volatile. Cela signifie qu'il ya une chance qu'une sous - classe pourrait atteindre et assign nullà in. Compte tenu de cette possibilité, «l'alias» est en fait là pour empêcher une condition de concurrence.

Considérez le code sans "alias"

private InputStream getInIfOpen() throws IOException {
    if (in == null)
        throw new IOException("Stream closed");
    return in;
}
  1. Thread A appelle getInIfOpen()
  2. Le thread A évalue in == nullet voit que ce inn'est pas le cas null.
  3. Le thread B est affecté nullà in.
  4. Le fil A s'exécute return in. Ce qui revient nullcar ac'est un volatile.

L '"alias" empêche cela. Maintenant, il inest lu une seule fois par le thread A. Si le thread B est affecté nullaprès le thread A, incela n'a pas d'importance. Le thread A lèvera une exception ou renverra une valeur non nulle (garantie).

Stephen C
la source
11
Ce qui montre pourquoi les protectedvariables sont mauvaises dans un contexte multi-thread.
Mick Mnemonic
2
C'est vrai. Cependant, AFAIK ces classes remontent à Java 1.0. Ceci est juste un autre exemple d'une mauvaise décision de conception qui n'a pas pu être corrigée par peur de casser le code client.
Stephen C
2
@StephenC Merci pour l'explication détaillée +1. Cela signifie-t-il que nous ne devrions pas utiliser de protectedvariables dans notre code s'il est multi-thread?
Madhusudana Reddy Sunnapu
3
@MadhusudanaReddySunnapu La leçon générale est que dans un environnement où plusieurs threads peuvent accéder au même état, vous devez contrôler cet accès d'une manière ou d'une autre. Cela peut être une variable privée uniquement accessible via setter, cela pourrait être un garde local comme celui-ci, cela pourrait être en rendant la variable en écriture une seule fois de manière threadsafe.
Chris Hayes
3
@sam - 1) Il n'est pas nécessaire d'expliquer toutes les conditions et états de course. Le but de la réponse est de montrer pourquoi ce code apparemment inexplicable est en fait nécessaire. 2) Comment ça?
Stephen C
20

Cela est dû au fait que la classe BufferedInputStreamest conçue pour une utilisation multi-thread.

Ici, vous voyez la déclaration de in, qui est placée dans la classe parent FilterInputStream:

protected volatile InputStream in;

Comme c'est le cas protected, sa valeur peut être modifiée par n'importe quelle sous-classe de FilterInputStream, y compris BufferedInputStreamet ses sous-classes. En outre, il est déclaré volatile, ce qui signifie que si un thread change la valeur de la variable, ce changement sera immédiatement reflété dans tous les autres threads. Cette combinaison est mauvaise, car cela signifie que la classe BufferedInputStreamn'a aucun moyen de contrôler ou de savoir quand inest modifiée. Ainsi, la valeur peut même être modifiée entre la vérification de null et l'instruction return in BufferedInputStream::getInIfOpen, ce qui rend effectivement la vérification de null inutile. En lisant la valeur de inune seule fois pour la mettre en cache dans la variable locale input, la méthode BufferedInputStream::getInIfOpenest sûre contre les modifications d'autres threads, car les variables locales sont toujours détenues par un seul thread.

Il existe un exemple dans BufferedInputStream::close, qui est défini insur null:

public void close() throws IOException {
    byte[] buffer;
    while ( (buffer = buf) != null) {
        if (bufUpdater.compareAndSet(this, buffer, null)) {
            InputStream input = in;
            in = null;
            if (input != null)
                input.close();
            return;
        }
        // Else retry in case a new buf was CASed in fill()
    }
}

Si BufferedInputStream::closeest appelé par un autre thread pendant l' BufferedInputStream::getInIfOpenexécution, cela entraînerait la condition de concurrence décrite ci-dessus.

Stefan Dollase
la source
Je suis d' accord, dans la mesure où nous voyons des choses comme compareAndSet(), CAS, etc. dans le code et dans les commentaires. J'ai également recherché le BufferedInputStreamcode et trouvé de nombreuses synchronizedméthodes. Donc, il est destiné à une utilisation multi-thread, même si je ne l'ai certainement jamais utilisé de cette façon. Quoi qu'il en soit, je pense que votre réponse est correcte!
sparc_spread
Cela a probablement du sens car il getInIfOpen()n'est appelé qu'à partir des public synchronizedméthodes de BufferedInputStream.
Mick Mnemonic
6

C'est un tel code court, mais, théoriquement, dans un environnement multi-thread, inpeut changer juste après la comparaison, de sorte que la méthode pourrait renvoyer quelque chose qu'elle n'a pas vérifié (elle pourrait renvoyer null, faisant ainsi exactement la chose à laquelle elle était censée prévenir).

acdcjunior
la source
Ai-je raison si je dis que la référence in peut changer entre le moment où vous appelez la méthode et le retour de la valeur (dans un environnement multi-thread)?
Debosmit Ray
Oui, vous pouvez le dire. En fin de compte, la probabilité dépendra vraiment du cas concret (tout ce que nous savons, c'est que cela inpeut changer à tout moment).
acdcjunior
4

Je pense que la capture de la variable de classe indans la variable locale permet inputd'éviter un comportement incohérent en cas de inmodification par un autre thread pendant l' getInIfOpen()exécution.

Notez que le propriétaire de inest la classe parente et ne la marque pas comme final.

Ce modèle est reproduit dans d'autres parties de la classe et semble être un codage défensif raisonnable.

Sam
la source