[gpz]

Just a note about what I read on the net: by OpenAL spec, OpenAL is thread-safe, so it must be safe to send commands to it from different threads.
Is it possible, that C# invoke itself is not thread-safe?

[Csantucci]

Dennis,
to avoid your first objection I have added in TrainCar.cs the test if the soundsource is still valid or not

if (OpenAL.alIsSource(soundSourceID))

just before working on it.
As it is now, I see only a possible uncovered case:
-sound process starts, finds the updater process flag free, and is interrupted before removing a sound source from SoundSourceIDs
-updater enters, works and is interrupted within the above if block
-sound process enters, and removes sound source from SoundSourceIDs and clears the sound source
-updater process enters and works now on a not existing sound source.
I think that the combination of these events is very unlikely.
I first had on the above "if" also a test if the sound process was in the critical section; this covered the above case, but caused short sound interruptions.
I am studying if and how to overcome the uncovered case.
A possibility is to split the work within the sound process in two: on an update SoundSourceIDs is updated removing the dead entries, and on a subsequent one the sound sources are cleared; in both updates a check is first done on the updater process flag.

I didn't understand your second objection.

[dennisat]

 Csantucci, on 08 February 2015 - 02:46 AM, said:

I didn't understand your second objection.

The update of sound source IDs in TrainCar.cs is working on the sounds for a particular car. The update of sound sources in the sound process is looping through all sound sources, at its own rate, with (normally) no synchronisation to any other processing of the owner of the source. Sound Trigger synchronisation is handled by an Events system and source Add / Remove by locks (which James requires to be replaced as soon as a reliable procedure is devised). It is therefore highly unlikely that the car sounds being processed at any one time by TrainCar.cs will coincide with those being processed in the sound process.

Dennis

[gpz]

I don't think, that possibly passing a non-existent (destroyed) sound source ID integer number to OpenAL would lead to a crash. It would mean OpenAL was very poorly coded, which is highly unlikely. So just by checking if these IDs are in sync, this problem would not be solved. I assume it is absolutely safe to pass a false ID to OpenAL.

[dennisat]

I don't think I've seen a crash actually in OpenAl. What we are crashing on are sound related lists that are being modified by one thread while another is trying to read / modify it at the same time. I'm sure all the crashes have been of the "Collection modified while being processed in a loop" type. This is a problem with our thread synchronisation, not OpenAl

Dennis

[Csantucci]

In the original code the list is converted to an array of integers in TrainCar.cs

            var soundSourceIDs = SoundSourceIDs.ToArray();

I would expect that, once the array of integers has been created, nothing around it can modify it indirectly.
So, there are two alternatives:
1) the above instruction is not executed in an uninterruptible way, and therefore it can be interrupted during creation of the array (e.g. by the sound process), so that, if the SoundSourceIDs list changes, when the above instruction is resumed unpredictable errors arise
2) the above instruction is executed in an uninterruptible way, and then problems are caused later in TrainCar.cs, when accessing a no more existing SoundSourceID.

Which one of the above is true?
The crash occurs later in TrainCar.cs in the OpenAl code (see my first post and the logfile in the bug report), but it is an out of memory crash, so it could be caused by both above problems
One could consider adding an Interlocked CompareExchange or similar things, as Dennis suggests, to the above instruction, if it is possible, maybe checking that the number of elements of SoundSourceIDs has remained the same before and after the creation of the array. I will add such a check.

Here something to read:
http://stackoverflow...list-collection

Excerpt from point 2 of the first answer: "Yes, iterating a copy of the list is always thread-safe, as long as you use a lock around the ToArray() method. Note that you still need the lock, no structural improvement. The advantage is that you'll hold the lock for a short amount of time, improving concurrency in your program."
But we can't use the lock...

[dennisat]

 Csantucci, on 08 February 2015 - 10:15 AM, said:

But we can't use the lock...

I would have thought that locks used on short blocks of code would do no harm to performance.

Dennis

[Csantucci]

OK, new release of patch
 Nosoundcrash1.zip (1.12K)
Number of downloads: 164
and new release of runactivities
 Runactivities.zip (1.22MB)
Number of downloads: 158

A protection has been inserted also around the generation of the array of soundsource IDs in TrainSet.cs. If the number of elements of the origin list has changed from before to after the generation of the array, the generation is repeated, up to 5 times (should really never happen, but it's correct not to introduce possible infinite loops).
The patch is "light": it does not change much with respect to presently available code, and has little overhead. So sound performance should remain substantially unaltered. The possibility of crashes due to concurrency should be fairly reduced (I hope), and for sure it's better than the actual status. Improvements can eventually be added later. I foresee to upload it tomorrow, after having read some further comment, if any.

[James Ross]

I'm not following in detail here but a few notes...

 Csantucci, on 08 February 2015 - 02:46 AM, said:

I think that the combination of these events is very unlikely.

Multi-threading makes the least likely things happen all the time. :sign_rockon:

 Csantucci, on 08 February 2015 - 10:15 AM, said:

Excerpt from point 2 of the first answer: "Yes, iterating a copy of the list is always thread-safe, as long as you use a lock around the ToArray() method. Note that you still need the lock, no structural improvement. The advantage is that you'll hold the lock for a short amount of time, improving concurrency in your program."

The lock is only need if you're modifying the collection in the other thread; if you're replacing it with a new collection (which is how most of the existing code in the world/scenery works, for example) you don't. Then the requirements are:

• You always copy the value to a local variable before doing anything with it.
  
• Nobody modifies the value in-place (as in, adding items to the List) - everybody must modify the value in a copy and replace the whole value in a single assignment (which is atomic).
  
• If multiple people are updating the collection, it must be done with a loop and Compare/Exchange (to avoid losing other code's changes).

 dennisat, on 08 February 2015 - 01:03 PM, said:

I would have thought that locks used on short blocks of code would do no harm to performance.

A lock which is never, ever contended (i.e. only used by one thread) would do a little harm. However, as soon as you have contention, you have very variable execution times as a contended lock - even a lightly contended one - incurs a huge delay during the blocking stage. This is normally fine for typical user-interactive apps (like the menu), since you don't notice if something blocks for 1ms; likewise, this is probably perfectly fine for the sound process itself. It is not fine for the renderer or updater processes, however, which need not just high performance (they have to do everything in less than 17ms) but high throughput (they have to do everything in less than 17ms every time).

[Csantucci]

James,
thank you for your post.
I would like to have this clarified better: you say:
"Nobody modifies the value in-place (as in, adding items to the List) - everybody must modify the value in a copy and replace the whole value in a single assignment (which is atomic)."
Referring to a list this would mean for the process modifying the list:
- make a local copy of the list (which is atomic, while generating an array from the list is not atomic)
- modify the local copy
- copy back the local copy to the original copy; (which is again atomic)
while for the process accessing the above list to operate on it it would mean
- make a local copy of the list (atomic operation)
- work on the local copy.

Did I understand correctly?

In our case the sound process adds/removes elements from the list and modifies what is linked to the list elements(it modifies the sound source parameters by OpenAL calls). The updater process does not add or remove elements from the list, but it modifies what is linked to the list elements (it modifies the sound source parameters by OpenAL calls).

Momentarily I have uploaded the above patch in release 2852, marking the bug report as "In Progress". Time will say if the patch heals the bug, as the bug appears quite seldom.