#356 Make libspectrum thread-safe

[Fredrick Meunier]

Make libspectrum thread-safe. Changes from BogDan Vatra in [#349].

[Fredrick Meunier]

I think this is a very valuable patch - I would very much like to use a thread-safe libspectrum on the Mac in a number of places.

Does anyone have any comments or concerns about intoducing this patch?

[Fredrick Meunier]

@BogDan: could you add some more detail about the status and requirements from this patch?

• Does the patch fully support both glib and replacement glib modes?
• What are the requirements of the methods that a library user can provide in libspectrum_mutex_vtable_t? Under what configurations are they required?
• Same for libspectrum_init_t? Are user_data and context used in any existing code or are they future proofing?
• Any other things you would like to point out?

[BogDan Vatra]

Sorry for the slow reply, somehow I didn't seen it until now.

1. Only the embedded glib.
2. libspectrum_mutex_vtable_t is used only by embedded glib to sync access to it's internal data. This https://github.com/bog-dan-ro/spectacol/blob/master/fuse/ui/qml/fuseemulator.cpp#L372 is how I'm using it in spectacol.
3. context is used eveywhere, it's needed to register the error notification for every context. user_data is passed to the error notification (on c++ will point to your class). Why I need it? Well, because I don't want silence the errors when I'm using libspectrum to sreach for screens while the emulator is active. This https://github.com/bog-dan-ro/spectacol/blob/master/fuse/ui/qml/spectrumscreen.cpp#L94 is how I'm using it in spectacol.
4. nothing that I can remember now :)

[Fredrick Meunier]

Thanks for the info BogDan.

I think this looks pretty good from reading and your description. I think I'd want to add a "standard" thread-enabled vtable before it went to the trunk, for fuse-utils etc. to be updated to use it too, and some updates to the libspectrum documentation but it looks OK to me as an initial patch given that real GLib already allows access to data structures from multiple threads as long as they are only manipulated by one so I think the fallback case is also reasonable.

• Are you manipulating libspectrum entities from multiple threads, or would it be enough for the myglib replacements to have a lock for their own data structure manipulations?
  • This may then keep the same interface as real glib which already has no shared state between GSList etc. instances but doesn't guard against corruption from manipulating a data structure from multiple threads.
• Why is struct libspectrum_tape moved from tape.c to tape_block.h?

I've slightly tidied the patch and comitted to branches/patches-356-threadsafe-libspectrum in [r5628].

[BogDan Vatra]

I had to move libspectrum_tape to header file, because I need to access the context filed from other .c files.

[BogDan Vatra]

Regarding the changes I've done to myglib replacements, they were needed to protect it's internal data i.e. GSList is accessing free_list & allocated_list global vars in may places which might come from multiple threads. As you can see in buff2Image all the calls are happening in one thread, but there are multiple threads that are calling this function in the same time (buff2Image is reentran).

That's why I stongly belive that C++ is a better choice for libspectrum & FUSE ;-) std containers don't need any synchronisation ;-) .

[Fredrick Meunier]

Thanks, I think the remaining todo list for this change to be in trunk is the following points barring other comments/issues that arise:

• Add a "standard" thread-enabled vtable
• Update fuse-utils etc. for the new API
• Update the libspectrum documentation to describe the standard thread safety features

[BogDan Vatra]

Updated libspectrum threadsafe patch on top of 1.2.1 branch.

[Fredrick Meunier]

Addressed with alternate approach from BogDan in libspectrum:merge-requests#2