Threading race condition bugfix for optimize_snr in pycbc_live

[xangma]

Problem:
There was a potential race condition in bin/pycbc_live where:

• if a coinc or single occurs,
• optimize_snr_checks == True,
• and the thread starts the optimize snr subprocess,

the main thread could have moved the analysis chunk on before the setup_optimize_snr function re-reads some data from self - the event class instance,. This causes lines like this: curr_data = self.data_readers[ifo].overwhitened_data(delta_f) to potentially read the wrong data.

Solution:
Split the setup_optimize_snr function, which normally accesses/modifies data in event, into two functions: setup_optimize_snr and run_optimize_snr. The setup function reads and saves all the data in the main thread before any GraceDB uploads or optimizing ensuring the correct data is saved before the main thread moves the analysis on. It returns the command to be run as a subprocess and the output directory. The run function takes these as inputs and runs after the event is uploaded.
One extra modification I had to include is because the setup_optimize_snr function was dependent on the gid from GraceDB, the run_optimize_snr function has to re-open and close the attributes hdf to add the gid in.

Happy for any suggestions/modifications. This is the best idea I've had since I found this was happening.

[xangma]

Is there any appetite for putting a comment in upload_in_thread stating something like this? :

#Warning: there be dragons ahead. Within threads created by the main thread, race conditions must be considered when accessing any data from unlocked objects that the main thread could be changing - e.g self.data_readers.

[tdent]

Seems like we could now use the MDC to test this, at least for a short time, given that it served its pre-O4 review purpose.

[GarethCabournDavies]

Note that as Xan is away at the moment, I will be working on this as well, so can no longer review!

[titodalcanton]

I plan to finally look into this next week.

[GarethCabournDavies]

Poke on this

[GarethCabournDavies]

Poke on this - this would be good to get in soon(ish)

[GarethCabournDavies]

@titodalcanton - is this in the pycbc live testing environment at the moment?

[titodalcanton]

No, only stuff on master is being tested on the MDC. This is now (again) near the top of my priority list.

[titodalcanton]

Typo: requried -> required. Also, there should probably be a if self.fu_cores is not None condition down below in setup_optimize_snr where fu_cores is used, otherwise I do not think the resulting command (--cores None) would be valid.

[titodalcanton]

I have no more comments, let's merge this and get it running on the MDC.