Violação de Acesso ao utilizar método Assign no Grupo de Medicamentos da NFe

[Clayton Alves]

Ao trabalhar com cópias de TMedCollection utilizando o método Assign da forma que está atualmente obtenho uma Violação de Acesso ao liberar minha cópia.

Isso acontece porque não existe implementado um método Assign na classe TMedCollection, o que faz com que a cópia utilize a referência de TMedCollectionItem e não uma cópia da instância.

Segue anexo código corrigido.

pcnNFe.pas

[Daniel Simoes]

Sua Unit está desatualizada em relação aos fontes atuais do SVN...

Mas o problema não parece ser esse pois

TMedCollection -> TACBrObjectList -> TObjectList -> TList

e em TList existe a implementação do método "Assign"

[Clayton Alves]

Sim, TList possui um método Assign que não cria cópias das Instâncias de TMedCollectionItem, causando o problema.

Em outras palavras, o Assign do TList não chama o Assign de cada TMedCollectionItem.

Realmente não estou com os fontes atualizados, mas basicamente o que está faltando é esta implementação:

procedure TMedCollection.Assign(Source: TMedCollection);
var
  I: Integer;
begin
  Self.Clear;
  for I := 0 to Source.Count - 1 do
    Self.New.Assign(Source.Items[I]);
end;

 

[Daniel Simoes]

Qual é o passo a passo para reproduzir o problema ?

[Clayton Alves]

var
  MedicamentosA: TMedCollection;
  MedicamentosB: TMedCollection;
begin
  MedicamentosA := TMedCollection.Create;
  MedicamentosB := TMedCollection.Create;

  MedicamentosA.New;

  MedicamentosB.Assign(MedicamentosA);

  MedicamentosA.Free;
  MedicamentosB.Free; // <-- Access Violation
end;

 

[BigWings]

É um comportamento herdado do TObjectList, quando você chama o Assign não são criados novos objetos, como o seu código faz, e sim simplesmente adicionados a referência a eles.

Pra não ter o AV, você pode fazer:

var
  MedicamentosA: TMedCollection;
  MedicamentosB: TMedCollection;
begin
  MedicamentosA := TMedCollection.Create(False);
  MedicamentosB := TMedCollection.Create;

Assim os elementos da lista serão destruídos apenas na chamada a MedicamentosB.Free;

[Clayton Alves]

2 hours ago, BigWings said:

É um comportamento herdado do TObjectList, quando você chama o Assign não são criados novos objetos, como o seu código faz, e sim simplesmente adicionados a referência a eles.

Pra não ter o AV, você pode fazer:

var
  MedicamentosA: TMedCollection;
  MedicamentosB: TMedCollection;
begin
  MedicamentosA := TMedCollection.Create(False);
  MedicamentosB := TMedCollection.Create;

Assim os elementos da lista serão destruídos apenas na chamada a MedicamentosB.Free;

Entendo. Mas o exemplo que dei foi bem simplificado propositalmente para deixar mais claro o problema.

No caso real, eu não instancio um TMedCollection. Eu tenho uma instancia de TDetCollectionItem, que por sua vez tem uma instância de TProd que por sua vez tem uma instancia de TMedCollection.

Neste cenário, utilizar o Assign de TDetCollectionItem com outro TDetCollectionItem e posteriormente realizar o Free irá disparar o AV da mesma forma.

 

 

[Clayton Alves]

O que estou sugerindo é que o método Assign seja implementado na classe TMedCollection assim como já foi implementado na classe TpagCollection que aliás segue a mesma hierarquia de herança (TACBrObjectList -> TObjectList -> TList).

[BigWings]

11 horas atrás, Clayton Alves disse:

No caso real, eu não instancio um TMedCollection. Eu tenho uma instancia de TDetCollectionItem, que por sua vez tem uma instância de TProd que por sua vez tem uma instancia de TMedCollection.

Neste cenário, utilizar o Assign de TDetCollectionItem com outro TDetCollectionItem e posteriormente realizar o Free irá disparar o AV da mesma forma.

O TDetCollection não tem a implementação do Assign, você teria um AV já no TDetCollection.

O mais indicado nesse caso seria realizar essa implementação no TACBrObjectList.

Só é preciso definir qual o comportamento esperado do Assign nessas classes:

- Criar novos objetos na nova lista e copiar o conteúdo.

- Seguir a implementação de TObjectList e apenas incluir a referência ao mesmo objeto, propagando o parâmetro de não destruir os itens para as classes filhas.

[Clayton Alves]

1 hour ago, BigWings said:

O TDetCollection não tem a implementação do Assign, você teria um AV já no TDetCollection.

O mais indicado nesse caso seria realizar essa implementação no TACBrObjectList.

Só é preciso definir qual o comportamento esperado do Assign nessas classes:

- Criar novos objetos na nova lista e copiar o conteúdo.

- Seguir a implementação de TObjectList e apenas incluir a referência ao mesmo objeto, propagando o parâmetro de não destruir os itens para as classes filhas.

@BigWings sim, o TDetCollection dispara o AV da mesma forma. Veja o projeto de exemplo em anexo.

Na minha interpretação, a semântica do método Assign é de Copiar e não de Mover.

Conforme consta na documentação do Freepascal:

"Assign copies the contents of Source to Self, if the classes of the destination and source classes are compatible."

E da embarcadero:

"Copies the contents of another, similar object."

Sim, as documentações falam do método Assign da classe TPersistent mas não estou me referindo a implementação e sim a semântica.

Exemplo que dispara o AV:

program Project1;

{$APPTYPE CONSOLE}

uses
  //FastMM4,
  SysUtils,
  pcnNFe;

var
  Det1, Det2: TDetCollection;
begin
  Det1 := TDetCollection.Create;
  Det2 := TDetCollection.Create;

  Det1.New;

  Det2.Assign(Det1); // Move algumas referências, quando na verdade deveria copiar.

  WriteLn('Liberando Det1');
  Det1.Free;

  WriteLn('Liberando Det1');
  Det2.Free; // <- Access Violation

  WriteLn('Fim');
end.

 

Project1.dpr

[Clayton Alves]

Mas só pra deixar claro, meu problema nem é com a TDetCollection (apesar de para mim estar claro que existe um problema na cópia) e sim com a classe TMedCollection que não implementa o Assign de modo a criar cópias do TMedCollectionItem.

[Daniel Simoes]

É uma interpretação... mas pense... se hoje os programadores entende que o Assign é para copiar os objetos (e não duplica-los), e se nós mudamos para ele duplicar...  Teremos um monte de usuários com MemoryLeak

Na implementação do FPC e Delphi, de Assign para o TList, você pode escolher qual o comportamento do Assign
 

Observe que o código padrão do Delphi/FPC de TList.Assign, é apenas Copiar a referência dos objetos da lista, e não duplica-los...

    procedure Assign(ListA: TList; AOperator: TListAssignOp = laCopy; ListB: TList = nil);

  TListAssignOp = (laCopy, laAnd, laOr, laXor, laSrcUnique, laDestUnique);

...
    // on with the show
  case AOperator of

    // 12345, 346 = 346 : only those in the new list
    laCopy:
      begin
        Clear;
        Capacity := LSource.Capacity;
        for I := 0 to LSource.Count - 1 do
          Add(LSource[I]);
      end;

 

[Clayton Alves]

24 minutes ago, Daniel Simoes said:

se hoje os programadores entende que o Assign é para copiar os objetos (e não duplica-los), e se nós mudamos para ele duplicar...  Teremos um monte de usuários com MemoryLeak

Especificamente no caso do TMedCollection não teriam memory leak já que o TMedCollection é uma TObjectList que é dona de seus itens, ou seja, os itens serão liberados junto com a coleção no momento da liberação.

Por fim o trecho abaixo resolveu meu problema de AV, mas é um "workaround" e acredito que a não duplicação desses itens possa ocasionar problemas no futuro.

   FProd.Assign(ItemXML.Prod);

   // Ajuste técnico
   FProd.med.OwnsObjects := False;
   FProd.med.Clear;
   FProd.med.OwnsObjects := True;

Agradeço o tempo de vocês.

Editado 17 Março, 2020 por Clayton Alves

[Daniel Simoes]

1 minuto atrás, Clayton Alves disse:

Por fim o trecho abaixo resolveu meu problema de AV, mas é um "workaround" e acredito que a não duplicação desses itens possa ocasionar problemas no futuro.

 Creio que não... afinal estamos seguinte o mesmo padrão que sempre existiu no TList e seus descendentes...