Opened 18 years ago

Closed 17 years ago

Last modified 17 months ago

#8066 closed patch (fixed)

Hack for IMuseGM threading problem

Reported by: SF/jamieson630 Owned by: SF/jamieson630
Priority: normal Component: Engine: SCUMM
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)

imuse.nonullplayer.diff (2.4 KB ) - added by SF/jamieson630 18 years ago.
Patch against a September 24 CVS snapshot
imuse.mutex.diff (5.7 KB ) - added by SF/jamieson630 17 years ago.
Patch against October 7 snapshot

Download all attachments as: .zip

Change History (9)

by SF/jamieson630, 18 years ago

Attachment: imuse.nonullplayer.diff added

Patch against a September 24 CVS snapshot

comment:1 by fingolfin, 18 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 SF/jamieson630, 18 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:3 by SF/ender, 17 years ago

Was any progress made on this? :)

by SF/jamieson630, 17 years ago

Attachment: imuse.mutex.diff added

Patch against October 7 snapshot

comment:4 by SF/jamieson630, 17 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 SF/jamieson630, 17 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 SF/jamieson630, 17 years ago

Owner: set to SF/jamieson630
Resolution: fixed
Status: newclosed

comment:7 by digitall, 17 months ago

Component: Engine: SCUMM
Game: Day of the Tentacle
Note: See TracTickets for help on using tickets.