Discussion :

[thibaut]

Bonjour,

Une petite question j'ai essayé d'implémenter un singleton avec la classe ThreadLocal malheureusement ça ne fonctionne pas correctement: plusieurs instances sont créées quand je lance une multitude de threads

Où est l'erreur. Merci pour vos lumières:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
public final class BeansPool {
  private static ThreadLocal<BeansPool> _singleton = new ThreadLocal<BeansPool>();
 
  /**
   * @return
   * @throws Exception 
   */
  public final static BeansPool getInstance() throws Exception {
      BeansPool pool = _singleton.get();
 
        if (pool == null) {
            System.out.println("New !!!");
            pool = new BeansPool();
            _singleton.set(pool);
        }
 
        return pool;
  }
}

A+

[BugFactory]

plusieurs instances sont créées quand je lance une multitude de threads

http://java.sun.com/j2se/1.4.2/docs/...readLocal.html

Ca a l'air d'être précisément ce à quoi sert ThreadLocal.

[thibaut]

cf. http://christophej.developpez.com/tu...tithread/#L3.3

[divxdede]

ThreadLocal est effectivement fait pour associer une valeur "potentiellement" differente à chaque threads de ton application.

[had35]

Apparamment, ThreadLocal n'est pas synchronisé.
L'instruction

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
 
private static ThreadLocal<BeansPool> _singleton = new ThreadLocal<BeansPool>();

requière un certain temps et deux threads peuvent l'appeler "simultanément", alors que l'autre n'a pas encore initialisé la variable. La variable est écrasée, mais si entre temps le premier thread a eu le temps de retourner l'objet qu'il a crée ...

Il faut synchroniser l'initialisation des singletons dans un contexte multithreading.

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
 
public final class BeansPool {
  private static ThreadLocal<BeansPool> _singleton;
 
  /**
   * @return
   * @throws Exception 
   */
  public final static BeansPool getInstance() throws Exception {
      synchronized(ThreadLocal.class) {
           singleton = new ThreadLocal<BeansPool>();
      }
      BeansPool pool = _singleton.get();
 
        if (pool == null) {
            System.out.println("New !!!");
            pool = new BeansPool();
            _singleton.set(pool);
        }
 
        return pool;
  }
}

[had35]

La synchronisation de mes threads n'est pas bonne car, évidemment, il faudrait plutôt quelque chose comme ça :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
 
synchronized(ThreadLocal.class) {
      if(singleton == null)
            singleton = new ThreadLocal<BeansPool>();
      }
}

Mais l'essentiel est ailleurs. J'ai plutôt l'impression que ThreadLocal fais exactement l'inverse du contrat Singleton, enfin si j'ai bien compris. Il retourne autant d'objet que de thread

[®om]

Pourquoi tu utilises ThreadLocal?

Pourquoi ne pas faire simplement:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
public final class BeanPool {
 
    private static final BeanPool instance = new BeanPool();
 
    private BeanPool() { ... }
 
    public static BeanPool getBeanPool() {
        return instance;
    }
 
}

[thibaut]

Certes de cette façon ça marche... mais ce n'est pas ce que je veux tester.

[had35]

Essayes un truc comme ça :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
 
public class TestThreadLocal {
	private static ThreadLocal<String> tl = new ThreadLocal<String>();
 
	public String getSingleton() {
		String s = tl.get();
		if(s == null) {
			s = Thread.currentThread().getName();
			tl.set(s);
		}
		return s;
	}
 
	public static void main(String[] args) {
		final TestThreadLocal test = new TestThreadLocal();
		for(int i=0; i<20; i++) {
			new Thread() {
				public void run() {
					System.out.println(test.getSingleton());
					System.out.println(test.getSingleton());
				}
			}.start();
		}
	}
}

[thibaut]

C'est grosso modo ce que je fais...

[le y@m's]

Pourquoi tu utilises ThreadLocal?

Pourquoi ne pas faire simplement:

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
public final class BeanPool {
 
    private static final BeanPool instance = new BeanPool();
 
    private BeanPool() { ... }
 
    public static BeanPool getBeanPool() {
        return instance;
    }
 
}

Tu as oublié de synchroniser la méthode static, ce qui est indispensable dans un contexte multithreads .

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
public final class BeanPool {

    private static final BeanPool instance = null;

    private BeanPool() { ... }

    public static synchronized BeanPool getBeanPool() {
        if(instance == null) {
            instance = new BeanPool();
        }
        return instance;
    }

}

[thibaut]

Non c'est faux, car quand tu fais :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

private static final BeanPool instance = new BeanPool();

Il semblerait que dans ce cas tu ne doives pas synchronizer ta méthode.

Testé et approuvé.

[le y@m's]

Non c'est faux, car quand tu fais :

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

private static final BeanPool instance = new BeanPool();

Il semblerait que dans ce cas tu ne doives pas synchronizer ta méthode.

Testé et approuvé.

Effectivement, j'ai lu un peu trop vite mea culpa.

[divxdede]

L'initialisation d'un singleton par une instanciation passive n'a pas besoin de synchronizer.

Ensuite tout dépends de ce que tu désires !! un singleton pour l'application ou un singleton par Thread ?

[thibaut]

Un singleton pour l'application.

[had35]

Un singleton pour l'application.

ThreadLocal n'est pas conçu dans cette optique. Comme je voulais le montrer avec le test, et qu'à bien perçu divxdede ,chaque thread obtient son propre singleton.

[le y@m's]

As tu essayé les deux solutions proposées par ®om et moi-même ?
D'ailleurs, celles-ci sont conseillées dans la conclusion de l'article dont tu as toi-même donné le lien

...
il est fortement recommandé d'accepter le coût de la synchronisation de toute la méthode ou d'utiliser un initialiseur static quand votre code le permet.

[thibaut]

Oui j'utilise la synchronization, mais c'était juste pour tester avec ThreadLocal car l'article http://christophej.developpez.com/tu...tithread/#L3.3

pouvait laisser à penser que l'utilisation de ThreadLocal est une alternative à la synchronization et en fait in n'en est rien (bien que dans d'autres articles les auteurs présentent l'utilisation ThreadLocal comme un bon palliatif au pattern DLC - DoubLock Checking), je suis donc revenu au bon vieux mutex des familles .

[divxdede]

Oui j'utilise la synchronization, mais c'était juste pour tester avec ThreadLocal car l'article http://christophej.developpez.com/tu...tithread/#L3.3

pouvait laisser à penser que l'utilisation de ThreadLocal est une alternative à la synchronization et en fait in n'en est rien (bien que dans d'autres articles les auteurs présentent l'utilisation ThreadLocal comme un bon palliatif au pattern DLC - DoubLock Checking), je suis donc revenu au bon vieux mutex des familles .

Si tu ne veux ni synchroniser l'ensemble du "getter", et non plus effectuer une initialisation passive, tu peux alors passer par un mutex simple du style

Code :

Sélectionner tout - Visualiser dans une fenêtre à part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
public class MySingleton
{
      private static Object lock = new Object(){};
 
      private static MySingleton instance = null;
 
      /* constructeurs privés */
      ...
 
      public static MySingleton getInstance()
      {
           synchronized( lock )
           {
                 if( instance == null ) instance = new MySingleton(...);
           }
           return instance;
      }
}

Mais comme le dit le document, cette solution n'apporte rien en terme de preformance vis a vis d'une syncrhonisation "complete" du getter.
C'est pourquoi on ne conseil que les 2x solutions:
- getter syncrhonisé
- initialisation passive du singleton

Avec une petite preference pour l'initialisation passive.

[thibaut]

Merci mais je sais tout ça