Discuss: Is it right to throw FmodInvalidHandleException when a channel has finished playing

[HakanL]

If I change the PlaySound sample to not loop and GetPosition or Stop is called after the sound has finished playing then I'm thrown a FmodInvalidHandleException. Obviously the handle is invalid (the channel has been stolen), but I wonder if it's correct to throw the exception, the C# code gets rather cluttered if we have to have a try/catch around all those just to see if the sample has finished. Suggestions?

[martindevans]

This is a difficult one - I don't want to change the semantics of the FMOD API too much, but this does make the code seriously ugly!

We could add an IsValid method to all the structs - then at least you can call that and check before using a handle. In fact while investigating how we'd implement this I found that FMOD already implements an isValid method so we should definitely do this!

I'll get on it

[martindevans]

Ok I just pushed up a commit which adds an IsValid method to all the structs. This should solve your problem :)

[HakanL]

That's good, but is there a concern with multi-threading/race-condition, if I call IsValid and it returns true, only to have the channel go stolen in the next clock cycle and my call to Stop() throws the exception? Part of me feels that calling Stop even on a stolen channel should just return, the purpose of Stop is met, the channel is stopped.

[martindevans]

Hmm yes that's a good point. I would guess that a handle will not go invalid until update is called, meaning this isn't a problem. I'm going to investigate the documentation now.

[martindevans]

The documentation makes no mention of an isValid method! It looks like this is a purely C# wrapper thing.

I think you're right, our best approach here is to suppress invalid handle exception when calling stop, but only stop for now.

[martindevans]

Ok I've implemented that. All Stop methods now suppress the ERR_INVALID_HANDLE message

[martindevans]

Had to re-open this issue. The IsValid method doesn't do what I thought it did. The internal implementation of IsValid is:

public bool isValid()
{
    return rawPtr != IntPtr.Zero;
}

However, it's perfectly possible for an Invalid handle exception to be thrown even when the pointer is not zero (I just tested this in more detail to check).

So the problem remains that we need some way of detecting and handling invalid handles which doesn't require all code to be wrapped in a try { } catch (FmodInvalidHandle) {}

[martindevans]

The confusion seems to have come about because there are two HandleBase objects, one for fmod and one for fmod_studio.

The fmod_studio HandleBase checks the validity of the pointer and calls an FMOD method to perform special checking for validity. The plain fmod HandleBase only checks the validity of the pointer.

For now I'm going to remove IHandle from all the types which would call fmod.isValid - this leaves us with these type having no isValid method:

• Channel
• ChannelGroup
• DSP
• DSPConnection
• Geometry
• LowLevelSystem
• Reverb3D
• Sound
• SoundGroup

[HakanL]

I was thinking, would it be better that the types that can become invalid from FMOD (Channel for example) have their methods return nullable results? For example GetPosition would return null if the channel is gone. That way you don't have to constantly check for exceptions in your consumer, and you can use the C#6 ? construct to check for null.

[HakanL]

It may be that a channel only becomes invalid at system.update, but we don't know that for sure, it would be good to not run into that risk, i.e. calling IsValid to have it return true and then GetPosition (or whatever) right after will throw InvalidHandle. I hate to see that :(

[martindevans]

It may be that a channel only becomes invalid at system.update

I checked this, that may be the case but unfortunately the IsValid method doesn't work as expected! For Studio types IsValid does what we want, but for these low level FMOD types IsValid is useless.

have their methods return nullable results

This is an interesting idea. I'd have to have a fiddle with that to see how ergonomic it is to use...

[HakanL]

We could call something simple like getPause in its IsValid method to see if the channel is invalid or not. But unless we know for sure there can't be a race condition I'm a little leery of risking an exception, if there is a race condition then of course by Murphy's law those conditions will only happen at the worst time.

[martindevans]

unless we know for sure there can't be a race condition I'm a little leery to risk an exception

I feel exactly the same. We should probably reach out to FMOD and ask them how exactly the API is meant to be used here, it seems like this would be a problem with this API in all languages...

[HakanL]

We can do that, but in C it's easier, you could just skip checking the result (or ignoring the invalid handle result code), I feel the issue is us raising the exception in C# and the extra work you have to do to handle that when consuming the library.

[martindevans]

Yes that's true. Perhaps the best way to do this is to have a flag on the structs which indicates if we should check for invalid handles...

var channel = Whatever();
channel.DoStuff();

//Now we're not sure when the channel will turn invalid, so we set the flag
channel.SupressInvalidHandle = true;

//And now doing this is totally safe!
Thread.Sleep(1000);
channel.Stop();

This is pretty much in line with the example code (c++):

//During setup, errors are being checked
result = system->playSound(sound[count], 0, true, &channel[count]);
ERRCHECK(result);
result = groupA->setVolume(0.5f);
ERRCHECK(result);

// Further down...

//Later on, when we're just interacting with channels, errors are not checked
bool mute = true;
groupA->getMute(&mute);
groupA->setMute(!mute);

In fact I think SSS is probably better here, because we can check all errors except Invalid_Handle, wheras the C++ code encourages you to simply skip all error checking!

[HakanL]

That makes sense for Stop(), but my concern is with calls like GetPosition, GetPause, etc

[martindevans]

Sorry I don't see the concern, surely once the suppress flag is set we're fine for any method?

[martindevans]

Looking at another example we have this:

result = channel->isPlaying(&playing);
if ((result != FMOD_OK) && (result != FMOD_ERR_INVALID_HANDLE) && (result != FMOD_ERR_CHANNEL_STOLEN))
{
    ERRCHECK(result);
}

So there's also FMOD_ERR_CHANNEL_STOLEN to suppress too. I think we should probably ignore those two errors by default and allow the user to toggle on checking for those errors if they wish.

[HakanL]

If we don't tell the user that the channel is dead when they call GetPosition (etc) then it would return position = 0 (or whatever is default for the return value). I figured we need to tell them that it's invalid. Maybe an overload that takes an out bool?

[martindevans]

Ah yes I see, which goes back to your suggestion of nullable results earlier. That certainly seems like the neatest solution to the problem so far.

So to summarise I think the best solution so far is:

• A flag to suppress error checking for FMOD_ERR_INVALID_HANDLE and FMOD_ERR_CHANNEL_STOLEN (true by default, set per struct)
• Return nullables from all queries

sound good?

[HakanL]

Nullables or overload with out bool invalidChannel? We also need a similar treatment to set-methods (SetPosition for example).

[martindevans]

Urgh, this is going to make the entire low level API extremely ugly. If we need to pass data out from a setter then we won't be able to use properties anywhere!

[HakanL]

Well, it will only affect the types that can be invalidated by FMOD, but I agree, it's ugly, just not sure how we should make it nicer? I guess we could ignore the error on the setters, no real harm I guess, and then have the consumer check elsewhere if it's dead.

[martindevans]

I guess we could ignore the error on the setters, no real harm I guess

Yep I was just thinking the same, that's the only way I can see to have a pleasant API. If someone is really concerned about making sure their setters are doing something useful they can set SupressInvalidHandle = false and now their setters will throw.

In fact we should probably have two flags (both set to true by default):

• Suppress FMOD_ERR_INVALID_HANDLE
• Suppress FMOD_ERR_CHANNEL_STOLEN

While it's conceivable you might want invalid handles to throw you'll almost never want channel_stolen to throw!

[HakanL]

I'm down with that. What about using nullables vs out bool on GetXX?

[martindevans]

I prefer nullables - it's easy to ignore an out parameter but you have to access the nullable.

[HakanL]

Sounds good to me, I just wanted to make sure we look at all options :)

[martindevans]

Alright looks like we've cracked it :D

I'm going to close this issue, and open up a new issue tracking these changes (since it's going to be a pretty large change).

[HakanL]

Good! :+1: