#8066 closed patch (fixed)
Hack for IMuseGM threading problem
| Reported by: | SF/jamieson630 | Owned by: | SF/jamieson630 |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Day of the Tentacle |
Description
The included patch fixes a threading problem that occurs under -ewindows during complex musical passages. This problem was reported under Bug #601926 (random crashes in the DOTT #2 cutscene).
Frequent clearing and reallocating of Player objects causes numerous uninit() calls for all the Parts belonging to the Players. These calls clear the _player pointer for each Part. Because of threading issues, the Part objects may still be trying to conduct operations at the time that uninit() is called -- and some of those operations require the use of the _player pointer.
This patch is a hack to make sure that the _player pointer is validated on-the-spot each time that it's used. If it has been removed, an appropriate default value is substituted in place of whatever was being loaded from the Player object.
Ticket imported from: #615737. Ticket imported from: patches/171.
Attachments (2)
Change History (9)
by , 22 years ago
| Attachment: | imuse.nonullplayer.diff added |
|---|
comment:1 by , 22 years ago
That's really a hack - it doesn't fix the problem at all, it only makes it much less likely to occur. The crash can still occur if the value gets changed after the check but before it is used.... Proper fix probably would involve using one/multiple mutexes in a clever way...
comment:2 by , 22 years ago
Fingolfin, agreed. And since this problem has only been reported from one system, the hack is not a "must have now" thing.
Back to work this week, but once I'm back off next week, I'll take a closer look at this thing. :)
"Threads create race conditions. Mutexes create starvation. Put 'em together and what have ya got? A program that's in a race to starve." (ancient Egyptian programmer saying)
comment:4 by , 22 years ago
I'm attaching imuse.mutex.diff, which guards any modifying sections of IMuseInternal (including the timer event) with a mutex. This makes IMuseInternal essentially a monitor (concurrent programming (n): a non-reentrant encapsulation of abstract resources) insomuch as most of IMuseInternal is now treated as one large critical region.
Because monitor constructs are not intrinsic to C++, this patch involves locking and unlocking mutexes all over the code. A cleaner solution, I'm thinking, might be to create a front-end class to handle the monitor functionality before calling the main class. This would separate the mutex code from everything else and make future changes easier to implement. It may also reduce the chance of the mutex getting accessed while already in the critical region -- something that is uncomfortably easy to do the way it's structured now.
Trinity, let me know if this patch also takes care of the crashing problem.
Also, anybody let me know if this thing causes total lockups in any circumstances. And comments on the monitor front- end idea would be greatly appreciated.
comment:5 by , 22 years ago
imuse.cpp version 1.74 (November 9, 2002) introduces IMuseMonitor, an improved mutex-based IMuse front-end that provides thread-safe access to both IMuseGM and IMuseAdlib. This approach is a much more elegant solution than the patch I submitted here.
I've directly committed the change because I believe it is important to get this in for release 0.3.0. During playtesting of Sam & Max, I ran into pointer corruption from IMuse threading issues frequently.
comment:6 by , 22 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:7 by , 6 years ago
| Component: | → Engine: SCUMM |
|---|---|
| Game: | → Day of the Tentacle |
Patch against a September 24 CVS snapshot