Problem 1007 - Multiple crashes in G4ParticleTable::Remove(G4particleDefinition *)
Summary: Multiple crashes in G4ParticleTable::Remove(G4particleDefinition *)
Status: RESOLVED FIXED
Alias: None
Product: Geant4
Classification: Unclassified
Component: particles (show other problems)
Version: 9.1
Hardware: Apple Mac OS X
: P5 normal
Assignee: kurasige
URL:
Depends on:
Blocks:
 
Reported: 2008-03-19 22:51 CET by Tom Roberts
Modified: 2008-03-21 19:51 CET (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this problem.
Description Tom Roberts 2008-03-19 22:51:19 CET
Both fIonTable and fShortLivedTable crash when removing an entry.

Both crashes are due to an invalid use of an iterator after calling std::vector<...>::erase() -- the iterator becomes undefined, and on Mac OS X it crashes the program (it may work on other implementations of the Standard Template Library). Two edits are needed:

In G4Iontable::Remove(G4ParticleDefinition* particle): insert a "break;" after "fIonList->erase(idx);"

In G4ShortLivedTable::Remove(G4ParticleDefinition* particle): insert a "break;" after "fShortLivedList->erase(idx);"

In both cases the loop over entries is exited, avoiding further use of the iterator. I'm pretty sure that each entry is entered only once into the vector, so this should be OK. This fixes the crash for me.


Comment:
While it may be unusual to remove particles from G4ParticleTable, the functions are implemented. I need to use them to avoid a huge memory leak when setting up to simulate multiple tracks in parallel: one G4ProcessManager is required per track, along with a copy of all its physics processes, and the only way to set that up is to call the user's ConstructProcesses() -- that loops over the G4ParticleTable, and to keep memory usage reasonable I must first reduce the particle table to just the particle of the current track (restore its contents immediately afterward).
Comment 1 kurasige 2008-03-20 03:23:38 CET
Thank you for reporting bugs.

I fixed G4Iontable::Remove() and G4ShortLivedTable::Remove() as your suggestion.
I looked over related methods of G4ParticleTable and found improper behaviours. So, I modified following methods at the same time.

G4ParticleTable::RemoveAllParticles
  do not delete IonTable and ShortLivedTable

G4ParticleTable::Remove
  delete the specified particle after removing it from the table
  (to make consistency with  G4ParticleTable::RemoveAllParticles method where all particles are deleted )

G4Iontable::Remove(), G4ShortLivedTable::Remove()
  just remove the particle from the table
  (should not delete it because the particle object is managed by G4ParticleTable)

Comment 2 Tom Roberts 2008-03-20 15:49:54 CET
>I fixed G4Iontable::Remove() and G4ShortLivedTable::Remove() as your
>suggestion.

Thanks.


> I looked over related methods of G4ParticleTable and found improper behaviours.
>So, I modified following methods at the same time.

>G4ParticleTable::RemoveAllParticles
>  do not delete IonTable and ShortLivedTable

I forgot this one. Yes, deleting those tables prevents the G4ParticleTable from ever being used again (which is why my workaround did not call RemoveAllParticles, but implemented its own loop calling Remove). Those tables should be deleted in the destructor (and only there).


>G4ParticleTable::Remove
>  delete the specified particle after removing it from the table
>  (to make consistency with  G4ParticleTable::RemoveAllParticles method where
>all particles are deleted )

I question the wisdom of this, in both Remove() and RemoveAllParticles().
First, it prevents the workaround I am using from working: I need to restore the particle table, and without the pointers to the particles remaining valid I cannot do that. Note that the documentation states that the particles are NEVER deleted. Remember that the user's code constructs the particles, and without the pointers remaining valid my code cannot restore the particle table. I cannot call the user's ConstructParticles() because I need just one particle, not all of them.

So this will destroy my current workaround to track particles in parallel. Note that at the Geant4 forum last week it was announced that multi-threading would probably be implemented in 2008, and they will need this, too. Unless some other method is implemented (there are better methods than my workaround; the best one requires a change to G4ProcessManager and also to EVERY G4Process).

The usual C++ rule is that whoever created an object should destroy it. While Geant4 code violates this in many places, it is precisely those places that cause problems (like this). I recommend NOT violating that rule, and letting the user's destructor destroy the particles (or not -- the resulting memory leak is small, and of no consequence as it comes immediately before program termination).


>G4Iontable::Remove(), G4ShortLivedTable::Remove()
>  just remove the particle from the table
>  (should not delete it because the particle object is managed by
>G4ParticleTable)

Yes.
Comment 3 Tom Roberts 2008-03-20 15:55:10 CET
It's worse than I thought. If the particles are deleted in G4particleTable::Remove()), then many already-created processes will cease to work because they keep a local copy of the pointer to the particle(s) they use.

PLEASE do not do that! There is a reason the documentation states the particles are NEVER deleted, and IMHO the code should comply. User code can delete them when they are no longer deleted; as user code created them this is appropriate; if the user code doesn't delete them, there are no major consequences as the memory leak occurs just before program termination.
Comment 4 kurasige 2008-03-21 05:05:40 CET
I understand your point.
Yes. It is a big problem in parallel tracking if particle objects are deleted.

Now the ParticleTable just keep the list of particles used in Geant4 and do not delete them in its destructor. Users are responsible to delete particle objects. So, I changed Remove and RemoveAllParticles methods to public in order to pass users responsibility of destruction of particle objects. I added DeleteAllParticles method to help users. (This method is not called in the destructor of the particle table) 

Comment 5 Tom Roberts 2008-03-21 19:51:10 CET
Thanks. Sounds good to me.

I have since found that this workaround works for geantino but not for real particles -- the energy loss processes need other particles (e, p) to be defined. So I am trying other workarounds for parallel tracking.