| Summary: | Several G4GeneralParticleSourceMessenger commands not working properly in multithreading mode | ||
|---|---|---|---|
| Product: | Geant4 | Reporter: | Christoph Terasa <cterasa> |
| Component: | event | Assignee: | Andrea Dotti <andrea.dotti> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | P5 | ||
| Version: | 10.1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Attachments: | example patch for 1744 | ||
Thank you for reporting this. I need some time to look at the issue. With 10.1.p01 UI commands related to GPS are a special case and need some attention. I will fix this asap. Andrea Created attachment 344 [details]
example patch for 1744
Patch which makes G4GeneralParticleSourceMessenger communicate with G4GeneralParticleSourceData instead of just the first GPS. For that purpose G4GeneralParticleSourceData now stores all known instances of G4GeneralParticleSource. Fixes several messenger commands handling sources.
The attached patch is just a proof-of-concept, it still needs some cleanup. I just wanted to show how delegating the communication between GPS and Messenger to the GPSData instance can solve the issue. Dear Christoph, Thank you for the report and the proposed patch. I've started to look at this issue and I think it should be easy to fix the issue and we probably need only a minimal change in the code (the point being removing in G4GeneralParticleSource the use of all local, and hence thread-private, variables and always use the shared GPSData). However I need some time to be sure I get all the cases. Can you please provide an example to reproduce the problem you have? Is it only /gps/source/clear UI command? Do you have other examples? Thank you for the patience, Andrea (In reply to comment #4) > Dear Christoph, > Thank you for the report and the proposed patch. > > I've started to look at this issue and I think it should be easy to fix the > issue and we probably need only a minimal change in the code (the point being > removing in G4GeneralParticleSource the use of all local, and hence > thread-private, variables and always use the shared GPSData). Exactly, I have since cleaned up my example code a bit locally, but that's the solution I came up with, too. > > However I need some time to be sure I get all the cases. Can you please provide > an example to reproduce the problem you have? Is it only /gps/source/clear UI > command? Do you have other examples? You can easily trigger the problem by using a macro like this /run/initialize /gps/source/clear /gps/source/add 1 /gps/particle e- /run/beamOn 100 This will properly clear the GPSData singleton, and the first GPS instance, but not the other instances of GPS. So the very fist gun will properly shoot electrons after that, but all other GPSses will shoot geantinos (or whatever is default in the PrimaryGenerator). I didn't really test it, but removing sources via messenger one by one will probably lead to similar problems. > > Thank you for the patience, > Andrea Hello, I have a prototype of a solution for this problem. If my testing are ok, I should be able to finalize it the next days. I was wondering if you would be willing to test the fix in your application. To do so, I would have to provide you a tarball that will replace the <g4source>/source/event directory, you would need to recompile G4 and test your code. Alternatively if you can provide me your application with some instructions, I could try the fix myself on your application. In such a case you can write to me privately at: adotti@slac.stanford.edu Regards, Andrea Dear Andrea, the project I currently use uses a lot of custom code and environment settings which I cannot easily share, and I'm currently a bit too busy to create a minimal example. If you could provide me with the tarball, or simply a patch, that would be great. Thank you, Christoph Candidate fix has been prepared. I'm in contact with original reporter to test the fix. If I receive confirmation of the fix working I will prepare a tag to event category. Andrea Tag event-V10-01-05 proposed for testing After additional feedback from a user a refinement to the fix is needed to prevent crashes in some cases. Without the refinement under development some UI commands acting on GPS following /gps/source/clear may crash the application if no new sources are added. With the refinement an error message will appear informing the user to explicitly add sources after a /gps/source/clear command. Tag: event-V10-01-06 proposed for testing |
When using multithreaded GEANT4 with the GPS, every thread gets its own ParticleGenerator, so every thread gets its own G4GeneralParticleSource. Due to changes in 10.1, the G4GeneralParticleSourceMessenger is singleton, as well as G4GeneralParticleSourceData. I will outline how the current version of G4GeneralParticleSourceMessenger leads to at least two major problems: First, every instance of G4GeneralParticleSource gets its first default source from G4GeneralParticleSourceData: G4GeneralParticleSource::G4GeneralParticleSource() : multiple_vertex(false), flat_sampling(false),normalised(false), theMessenger(0) { GPSData = G4GeneralParticleSourceData::Instance(); currentSource = GPSData->GetCurrentSource(); currentSourceIdx = G4int(GPSData->GetSourceVectorSize() - 1); [...] so they all share the same default source. The problem now is that (for example) the /gps/source/clear command (and some others) in the messenger are not properly called for all instances of G4GeneralParticleSource, but only for the one in the first thread, because of the constructor of the messenger: G4GeneralParticleSourceMessenger::G4GeneralParticleSourceMessenger (G4GeneralParticleSource *fPtclGun) : fGPS(fPtclGun),fParticleGun(0),fShootIon(false), [...] else if( command==clearsourceCmd ) { fGPS->ClearAll(); } which calls ClearAll only for that first G4GeneralParticleSource: void G4GeneralParticleSource::ClearAll() { currentSourceIdx = -1; currentSource = 0; GPSData->ClearSources(); normalised=false; } This correctly sets the source to 0, and also clears all sources form G4GeneralParticleSourceData singleton. In the G4GeneralParticleSource singleton the sources are removed by simply clearing the sourceVector containing the sources: void G4GeneralParticleSourceData::ClearSources() { currentSourceIdx = -1; currentSource = NULL; sourceVector.clear(); sourceIntensity.clear(); } This does *not* call the destructor on the elements in the sourceVector, so the sources still exist. Incidentally, due to the constructor outlined above, all other G4GeneralParticleSource instances in the other threads still point to the source which got cleared form the vector, and do *not* have their currentSource and currentSourceIdx variables value reset, because their ClearAll() methods have never been called. This leads to two problems: 1) from my tests, the source is now longer reachable from the messenger and cannot be modified 2) if the default source is deleted via /gps/source/clear, threads after the first still all have a default source, while the very first thread and G4GeneralParticleSourceData don't have any sources any more. Adding a new source via messenger now creates a *second* source for all threads except the first, and a new *first* source for the first thread and G4GeneralParticleSourceData. My proposed fix would be to make sure that the Messenger properly calls GPS methods for *all* instances of G4GenralParticleSource. For that purpose it would be beneficial for G4GeneralParticleSource to have a static vector of all instantiated GPSes, or have G4GeneralParticleSourceData store that information, or have a dedicated G4GeneralParticleSourceManager to access all instances.