search

astrorama / SourceXtractorPlusPlus

SourceXtractor++, the next generation SExtractor
https://astrorama.github.io/SourceXtractorPlusPlus/
GNU Lesser General Public License v3.0
72 stars 9 forks source link

Fix a race condition when setting/getting properties #480

Closed ayllon closed 2 years ago

ayllon commented 2 years ago

Fixes #478

marcschefer commented 2 years ago

Our strategy for multithreading is that sources are always handled in a single thread. It has been our guiding principle since the beginning so we should never need a mutex in them.

If that principle has been broken we should understand why and fix that instead. And if that is not viable we need to reconsider our entire multithreading approach but that would need to be well justified.

ayllon commented 2 years ago

In #478 I have commented on another option. Basically it would be adding a pipeline interface of sorts where ownership is transferred between stages. Right now the observer pattern does not distinguish between a notification (check images) and someone else taking responsibility of the object (prefetcher or measurements).

marcschefer commented 2 years ago

I see the problem is that we have the checkimage generation done in the main thread after the source has been sent to be transferred to another thread.

We just need to change the order so that the checkimages are processed first. We could just call the addObserver first as the order is preserved. That would fix it but there would still be a danger of getting it wrong in the future.

I agree that a pipeline interface that can have only one destination and is always called last would be best.

mkuemmel commented 2 years ago

The line causing the problem (according to #478): https://github.com/astrorama/SourceXtractorPlusPlus/blob/eb09a460f4d2702c85c890612713e556f3354d7b/SEImplementation/src/lib/CheckImages/DetectionIdCheckImage.cpp#L34 was introduced with the MEF detection. Maybe something can be changed there?

marcschefer commented 2 years ago

was introduced with the MEF detection. Maybe something can be changed there?

Yes before we used only properties that were already computed but DetectionFrameInfo needs to be "computed" (even though it's a trivial). So that's the difference, in a way we were just lucky before but the design flaw existed already.

ayllon commented 2 years ago

Closing this one and going for the Pipeline interface.