Opened 17 years ago

Closed 17 years ago

Last modified 13 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 17 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, 17 years ago

Attachment: imuse.nonullplayer.diff added

Patch against a September 24 CVS snapshot

comment:1 by fingolfin, 17 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, 17 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, 13 months ago

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