Discussion :

[Mozofeuk]

Bonjour à tous,

J'ai pu parler rapidement de cela sur la page Facebook Silverlight il y a quelque temps et depuis je me pose des questions car j'avoue être un peu embrouillé par tout ça.


Prenons un exemple qui pour moi apparaît souvent dans mes codes, et qui après tout ce que j'ai pu lire (et essayer de comprendre) pourrait être une mauvaise pratique.
Ici on detecte un appuis sur un bouton permettant de delete un objet, avec bien sur, demande de confirmation à l'utilisateur

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
26
27
28
29
30
31
32
33
34
35
36
37
38
39
private void Bt_Delete_Click(object sender, RoutedEventArgs re)
            {
                if (SelectedObject !=null)
	            {
	                //Ask confirmation to the user
                    ConfirmChildWindow _confirmChildWindow = new ConfirmChildWindow("Are you sure to Delete the object ?");
                    _confirmChildWindow.Closed += (o, e) =>
                    {
                        //If yes
                        if (_confirmChildWindow.DialogResult == true)
                        {
                            //Remove it to the DBContext
                            DBContext.Objects.Remove(SelectedObject);
                            DBContext.SubmitChanges().Completed += (o1, e1) =>
                            {
                                //And then when it remove we reload all the Object
                                //Load bar during the load of the Objects
                                Pb_LoadingObject.Visibility = Visibility.Visible;
 
                                //load all the Object 
                                LoadOperation _ObjectLoadOperation = DBContext.Load(DBContext.GetObjectQuery(SelectedOtherObject.ID));
                                Cb_Object.ItemsSource = _ObjectLoadOperation .Entities;
 
                                _ObjectLoadOperation .Completed += (o2, e2) =>
                                {
                                    Pb_LoadingObject.Visibility = Visibility.Collapsed;
                                    InfoChildWindow _infoChildWindow = new InfoChildWindow("The Object has been deleted !");
                                };
                            };
                            UnCheckAllCheckBox();
                        }
                         //if not
                        else
                        {
 
                        }
                    };
                }
            }

On voit donc ici que lors d'un click sur le bouton, on demande la confirm à l'utilisateur => 1ère lambda pour le Closed, puis on le remove du DBContext => 2éme lambda pour le submit Completed.

Pour mon DBcontext à la rigueur, comme il est déclarer dans ma page, sa durée de vie de vie dépasse la durée de fonction elle même si j'ai bien compris, donc cela ne pose pas trop de problème pour la lambda avec ma confirmChildWindow, j'ai vu ici que cela pouvais poser des problèmes.

Alors je me dis qu'avec tout ça, mon "enchevêtrement" de lambda genre poupées Gigognes été peut être quelque choses à proscrire non ??
Bref je suis un peu perdu dans tout ça et j'attends vos avis d'expert

[antoine.debyser]

Bonjour,

La fuite en question est un classique de .NET :
- Abonnement à un événement sans se désabonner.

Le problème, c'est que l'abonné et le lanceur d’événement se maintiennent mutuellement en vie, et donc le garbage collector ne peut rien faire.

La solution se désabonner, oui mais toi tu ne peux pas (directement), car le delegate que tu a fournit pointe sur une méthode anonyme.
Une solution consiste à stocker le delegate dans une variable, comme ça tu peux te désabonner.

Une autre solution consiste à ne pas utiliser des méthodes anonymes pour ca, et donc par extension des lambda.
Enfin une autre solution consiste à utiliser des références faible. Cette dernier est là plus difficile à mettre en oeuvre.

[Mozofeuk]

Ne pas utiliser d'expression lambda me gène un peu car j'aime bien la manière dont ça permet de formater le code (sans renvoyer à des fonctions différentes à chaque fois) je trouve que l'on gagne en visibilité.

Tu pourrais me donner plus de détails/sources sur la manière de :

-Stocker les delegates pour pouvoir se désabonner

-utiliser des références faible

En tout cas merci !

[DonQuiche]

C'est pour cela que je considère que les événements "uniques" (closed, completed) sont un mauvais design. Pour moi, ici, DBContext.SubmitChanges() devrait accepter un callback. Une fois les changements complétés, la liste des callbacks serait vidée.

J'ai l'exemple d'une ressource de ce genre dans un code sur lequel je bosse en ce moment, avec une méthode BeginLoad. Plutôt que de fournir un événement loaded, la méthode accepte un callback et garantit que celui-ci sera toujours appelé, même si le chargement était complété quand l'appel à BeginLoad a eu lieu (BeginLoad peut avoir été appelé au préalable).

Le problème est moindre avec les événements récurrents (SizeChanged, etc) parce que les abonnés le sont en général pour la durée de leur vie entière.

[antoine.debyser]

C'est pour cela que je considère que les événements "uniques" (closed, completed) sont un mauvais design. Pour moi, ici, DBContext.SubmitChanges() devrait accepter un callback. Une fois les changements complétés, la liste des callbacks serait vidée.

J'y avais jamais fait gaffe, mais en effet, c'est une bonne méthode. Elle force l'utilisateur de ta classe à ne pas induire de fuite mémoire.

Tu pourrais me donner plus de détails/sources sur la manière de :

-Stocker les delegates pour pouvoir se désabonner

-utiliser des références faible

Pour les références faible c'est compliqué. Il n'y a pas d'objet générique sur WeakReference actuelement dans le framework, je crois avoir lu que cela aller changer en .net 4.5.
Je ne peux que te renvoyer à un article que j'ai en stock dans mes favoris

Pour le delegate, ca devrait ressembler un peu près à ç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
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
private void Bt_Delete_Click(object sender, RoutedEventArgs re)
{

    //TOTO est a remplacer par le vrai type du delegate
    Toto maCallback;

    if (SelectedObject !=null)
    {
        //Ask confirmation to the user
        ConfirmChildWindow _confirmChildWindow = new ConfirmChildWindow("Are you sure to Delete the object ?");
        maCallback = (o, e) =>
        {
            //If yes
            if (_confirmChildWindow.DialogResult == true)
            {
                //Remove it to the DBContext
                DBContext.Objects.Remove(SelectedObject);
                DBContext.SubmitChanges().Completed += (o1, e1) =>
                {
                    //And then when it remove we reload all the Object
                    //Load bar during the load of the Objects
                    Pb_LoadingObject.Visibility = Visibility.Visible;

                    //load all the Object 
                    LoadOperation _ObjectLoadOperation = DBContext.Load(DBContext.GetObjectQuery(SelectedOtherObject.ID));
                    Cb_Object.ItemsSource = _ObjectLoadOperation .Entities;

                    _ObjectLoadOperation .Completed += (o2, e2) =>
                    {
                        Pb_LoadingObject.Visibility = Visibility.Collapsed;
                        InfoChildWindow _infoChildWindow = new InfoChildWindow("The Object has been deleted !");
                    };
                };
                UnCheckAllCheckBox();
            }
             //if not
            else
            {

            }
            
            //Ici on va se desabonne
            o.Closed -= maCallback;
        };
        
        //Ici on s'abonne
        _confirmChildWindow.Closed += maCallback;
    }
}

[DonQuiche]

Pour le delegate, ca devrait ressembler un peu près à ça.

Petit précision à l'attention de Mozofeuk : ici Antoine a stocké le lambda dans un délégué. Mais si ça n'avait pas été un lambda, on n'aurait pas été obligé de stocker le délégué quelque part, on aurait très bien pu faire :

Code :

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

1
2
someObject.SomeEvent += new SomeHandler(OnSomeEvent);
someObject.SomeEvent -= new SomeHandler(OnSomeEvent);

[Mozofeuk]

Ok merci à vous deux !

Donc pour résumer, si je fait la méthode d'antoine (car je préfère continuer à utiliser les lambda) pour tout mes évènements "uniques" (closed, completed), je limite quasiment tous les problèmes de memory leak ?

[DonQuiche]

Donc pour résumer, si je fait la méthode d'antoine (car je préfère continuer à utiliser les lambda) pour tout mes évènements "uniques" (closed, completed), je limite quasiment tous les problèmes de memory leak ?

Oui, il n'y aura plus de problème.

Enfin, à deux conditions :
* L'événement doit bien être appelé (si tu t'abonnes alors que l'événement a déjà été invoqué, le code ne sera pas exécuté et donc le désabonnement non plus)
* Gaffe aux exceptions, il faudrait placer le désabonnement dans un finally.

[Mozofeuk]

Ok merci pour tout

[antoine.debyser]

Enfin, à deux conditions :
* L'événement doit bien être appelé (si tu t'abonnes alors que l'événement a déjà été invoqué, le code ne sera pas exécuté et donc le désabonnement non plus)
* Gaffe aux exceptions, il faudrait placer le désabonnement dans un finally.

Exacte.
Du coup je serait curieux de voir ce qu'il se passe, quand en plus, on utilise une lambda pour gérer la fermeture de la fenêtre.
Comme je suis pas sur d'être clair, un exemple

Code :

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

1
2
3
4
5
6
7
8
private void Bt_Delete_Click(object sender, RoutedEventArgs re)
{
   // ... //
  Closed += (o, e) => 
  {
     _confirmChildWindow.Closed -= maCallback;
  }
}

J'ai doute sur la capacité du garbage collector à supprimer l'instance de la fenêtre dans ce cas.

[Mozofeuk]

Oui du coup il faut forcement se désabonner en dehors de la lambda elle même c'est ça ?

Code :

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

1
2
3
4
5
6
7
8
9
private void Bt_Delete_Click(object sender, RoutedEventArgs re)
{
   // ... //
  Closed += (o, e) => 
  {
   //Todo
  }
  _confirmChildWindow.Closed -= maCallback;
}

[antoine.debyser]

En fait ça devrait aller.
Car c'est "A" qui contient la référence de "A". Là normalement le garbage collector sait qu'il peut supprimer "A", si personne d'autre ne connait "A".

Le code, que tu montres, est erroné.
Car dans le cas présent tu t'abonne pour te désabonne de-suite.

Oui du coup il faut forcement se désabonner en dehors de la lambda elle même c'est ça ?

Il faut bien comprendre une chose avec les lambda et plus généralement avec les méthodes anonymes. C'est la notion de "closure".

Petit exemple tout bête:

Code :

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

1
2
3
4
5
6
7
8
9
List<Action> actions = new List<Actions>();
for(int i = 0; i < 10; ++i)
{
   actions.Add(new Action(() => { Console.Writeline("i : " +i); }));
}
foreach(Action action in actions)
{
   action();
}

Ceci n'affichera pas 0,1,2, ..., 8,9 mais 10,10,10 ....

Ça montre que dans ce cas le compilateur utilise une référence de i dans la méthode anonyme généré.
Et c'est exactement pour cela que le code tes premiers postes marche. Le compilateur s'arrange pour conserver la référence à "_confirmChildWindow"

Et on utilise le même principe pour ce désabonné.

J'ai doute sur la capacité du garbage collector à supprimer l'instance de la fenêtre dans ce cas.

Le doute que j'avais été lié "aux références circulaire", mais le garbage gère très bien cela.
Désolé de t'avoir induit en erreur.