PabloMK7 / citra

A Nintendo 3DS Emulator
GNU General Public License v2.0
3.62k stars 618 forks source link

AsyncWakeUpCallback cannot be serialized #28

Open CasualPokePlayer opened 7 months ago

CasualPokePlayer commented 7 months ago

Is there an existing issue for this?

Affected Build(s)

Since 9ec4954380265851b6f37cbf29b9e28d8ae81683

Description of Issue

The issue comes in multiple layers.

  1. BOOST_CLASS_EXPORT_KEY/SERIALIZE_EXPORT_IMPL/SERIALIZE_IMPL macros were not used for this class. These are needed for boost serialization to properly work and actually have serialization methods instantiated. If BOOST_CLASS_EXPORT_KEY/SERIALIZE_EXPORT_IMPL is not used, you will get a crash on attempting serialization. If SERIALIZE_IMPL is not used, you will not get the serialization methods compiled, and thus not actually get any kind of compile time errors from improper serialization code.
  2. AsyncWakeUpCallback is a template class, meaning technically multiple different versions of AsyncWakeUpCallback exist, so in order to use BOOST_CLASS_EXPORT_KEY/SERIALIZE_EXPORT_IMPL/SERIALIZE_IMPL each possible kind of instantiated class have to have the macros used on them. However, that is problematic, as the type used in this template class is a lambda, which does not have a defined type in the first place.
  3. These lambdas have captures, which would also need to be serialized (after all, in order to deserialize these, you would need to be able to recreate these lambdas from the serialized data, and that includes the captured data). Of course, this isn't actually possible to do with lambdas.

The fix likely would be to rework the RunAsync system to not take in a lambda, but rather take in a functor class implemented for each usage of RunAsync, each with serialization methods implemented.

Expected Behavior

AsyncWakeUpCallback being serialized should work and not crash.

Reproduction Steps

This is a bit hard to reproduce, as it relies on AsyncWakeUpCallback being put into a ThreadCallback at some point and that callback is not reached by the end of a frame, and therefore is still in the waiting_threads list. Pounding the savestate key when the game is constantly reading rom eventually causes the crash.

Log File

N/A

System Configuration

N/A

PabloMK7 commented 7 months ago

Actually, I can't think of any use cases of RunAsync that would make sense to have its state serialized, as they depend on the state of the host machine. It may be more reasonable and simpler to prevent save states if any async operations are pending.

CasualPokePlayer commented 7 months ago

The primary case where this is a problem is the File::Read method (most common case people would run into), where the result_function actually performs the writing into the MappedBuffer. The captured async data holds the data that would be written into at the end of the async operation.

Noting here, the problem isn't so much the async section (even if really_async is false or the promise is finished you still have an issue), but rather the result function, which may be delayed for some time from SleepClientThread. SleepClientThread/ThreadCallback is also not something only used for async operations, so I'm not quite sure if you can really detect if some async operation is in the event queue.

Noting to the serialization code does attempt to wait for the promise to finish before it actually does seralize, so the async section lambda shouldn't need any serialization (as it's all finished), it's only the result lambda which finalizes the async operation on the emulator thread (which might be delayed some time into the future) needs serialization.

CasualPokePlayer commented 7 months ago

Looking at the sections RunAsync is called I'm seeing what you mean, most of them have responses which are just based on some OS specific responses (e.g. socket responses), so fairly useless to serialize (and the output data might be garbage too after a state load). Probably fair enough to try to avoid savestates during these operations (although I imagine ROM reads might make that annoying as that's probably not too uncommon).