static PaError StopStream( PaStream *s ) always returns success

[RSAsterix]

I've recently started building with more verbose warnings, and it flagged something concerning:

portaudio/src/hostapi/dsound/pa_win_ds.c

Line 3038 in aa1cfb0

static PaError StopStream( PaStream *s )

This function doesn't change PaError result. It also does nothing with HRESULT hr.
That means it almost always returns paNoError.

As the FIXME's imply, it should probably indicate when IDirectSound(Capture)Buffer_Stop fails.

@dechamps and @RossBencina recently touched this function, could you have a look at it?
It should probably do something like
return hr == DS_OK ? paNoError : <some error code>
Or it should set result to something error-ish (instead of hr)

[RossBencina]

Thanks for raising this, it's good to track this in an issue.

From my point of view the problem is that the PortAudio API is underspecified with respect to whether Pa_StopStream can return an error. And if it can return an error, and does return an error, what state the stream is expected to be in. Step one is to clarify this.

This is related to "destructors can't throw exceptions" in C++.

EDIT #1: Phil and I were discussing this. An error inside StopStream or CloseStream could mean many things, with many destination states. With respect to StopStream:

• Error occurred, the dsound object is definitely still running, you can try again later. e.g. a bad parameter was passed
• Error occurred, the dsound object is definitely stopped e.g. there was a resource cleanup issue, but the actual stream is stopped.
• Error occurred, who knows what state the dsound object is in ("unknown stream state").

EDIT #2:

If "destructors can't throw exceptions" StopStream should always succeed, and CloseStream should always succeed, irrespective of whether the native API returns an error.

If the native API does return an error either the native API is violating "destructors can't throw exceptions", or the native API always succeeds and is just returning an informational error. I don't think we can known in general which of those cases it is on all host APIs.

My view is that StopStream and CloseStream should always succeed provided that the user callback returns in a timely fashion. So I'm not sure that it makes sense for them to return an error.

@dechamps has previously raised the point that if the PA stream transitions to stopped (or closed), but the underlying stream is in an unknown (still running) state, and the user unloads a DLL containing the user stream callback, then the program will crash. The mitigation for that would be to ensure that the callback has been zeroed and is not active prior to returning from StopStream, even if an error occurs during StopStream. If the callback hangs (e.g. due to deadlock or infinite loop), then it might be reasonable for StopStream to hang (the alternative would be to return an error that indicates that the stream is still running, but that violates the "destructors can't throw exceptions" policy. The problem remains if the user wants to unload the PortAudio DLL (in that case we'd need to construct dynamic thunks for the callbacks that stay resident after PortAudio DLL is unloaded).

[RossBencina]

related #919, #864, #115

[dechamps]

My two cents:

In general, function calls should be atomic. That is:

• If a function returns OK, the caller can assume everything was successful and the program state is as documented for a successful call.
• If a function returns an error, the caller can assume nothing was done and the program state is exactly as it was before the call (or at least not noticeably different).

"Half error" states such as "we closed the stream but we couldn't release some resource" are a bad idea and will not end well, unless they are well-documented and the API is phrased in such a way as to make it hard for callers to get confused (for example, they could get reported through some side channel).

As a side note, I don't think it is a good idea to have a "close" or "stop" function return an error code because that is unactionable: as a caller, what are you supposed to do when a close function fails?

1. You could retry the call, but that seems pointless as there's no reason to believe it won't fail again in the exact same way.
2. You could ignore the error, but then you end up in some kind of wedged state where you can't stop/close things, which likely means more trouble down the line.
3. You could crash the entire process immediately, on the grounds that failing to close something is never supposed to happen and is therefore indicative of corrupt program state or a serious bug (undefined behavior, basically). Crashing prevents potential future corruption (which may be worse than the crash itself) and makes the problem very obvious at the point it happens, thus easy to troubleshoot.

At the risk of sounding controversial (again), I advocate for (3) being the only reasonable option, and for that reason I believe PortAudio should not even attempt to return error codes from stop/close functions; instead, it should assume a serious error has occurred (stop/close functions are never supposed to fail), and crash the process immediately to prevent further corruption.

[RSAsterix]

@dechamps If I understand you correctly (which I might not), I would have to strongly disagree with your last remark.

If PortAudio starts crashing the entire process because it can't close some resource, it becomes completely unusable and we'll have to stop updating it.

We're trying to build an application that's as stable as possible. The one thing it cannot do is stop playback. Ever.
So imagine you've got two active outputs and you're trying to close one of them, that fails, and the entire process crashes (losing the stream you didn't touch as well).
That's utterly unacceptable.

In this case, I wouldn't mind if (several of the) IDirectSound(Capture)Buffer_Stop calls fail. You could generate a debug message?

The only issue I had here, was that it seemed that the results of the calls were stored to be returned, but weren't.

[dechamps]

If PortAudio starts crashing the entire process because it can't close some resource, it becomes completely unusable and we'll have to stop updating it.

I agree. But the way to avoid this is not to paper over the problem and sweep it under the rug. The way to avoid this is to root cause why it can't close that resource and fix that. Failing to close a resource should never happen and is indicative of a bug. The correct course of action is to fix the bug, not hide it.

[RSAsterix]

Failing to close a resource should never happen and is indicative of a bug. The correct course of action is to fix the bug, not hide it.

So if I understand correctly, the very least this particular function could do is have some stderr (debug) output?

[RossBencina]

If PortAudio starts crashing the entire process because it can't close some resource, it becomes completely unusable and we'll have to stop updating it.

You have asked @dechamps opinion but please don't expect it to represent project policy. PortAudio is not going adopt a policy of crashing, I can assure you of that.

[RossBencina]

Failing to close a resource

A native function is being called by PortAudio and that function is returning an error. That's all we know. We don't know anything about the cause of the error. We have limited control over the function that is returning the error. Depending on the native documentation, we may or may not know the state of the resource after receiving the error.

should never happen

This is an incorrect assumption. For a start, the type signature of the native API function under consideration indicates that it can return an error, so clearly the API designers thought that there were potentially circumstances where it could return an error. After all, "destructors don't throw exceptions" isn't a universal policy. Typically we don't know enough about the inner workings of drivers and native APIs to make categorical statements like "should never happen". E.g. what happens if you try to close a device that's been unplugged? Does the native API return an error? who knows.

and is indicative of a bug.

It might be indicative of a bug. It could be that a non-bug aspect of the native error model caused an error to be returned. Even if it is indicative of a bug, the bug could be in PortAudio, or it could be somewhere in the native audio stack, down to the driver. Obviously if it is a bug in PortAudio it should be fixed. But our assertion handling policy (#604) already dictates that we should not fail with an assert in the case of an unexpected return value from a native function (e.g. we do not want an assertion failure if the native API returned an error that was not caused by PortAudio).

The correct course of action is to fix the bug, not hide it.

If there is a bug, and its a bug in our code, then we can fix it, otherwise we can't fix it. But this isn't a discussion about software development principles it's about how should a particular build of PortAudio handle unexpected errors at runtime. Fixing the bug is not an option at runtime. Failing with an assert (or equivalent crashing) is not how we are going to handle it as was already decided in #604

So if I understand correctly, the very least this particular function could do is have some stderr (debug) output?

Sounds good.

[dechamps]

A native function is being called by PortAudio and that function is returning an error. That's all we know. We don't know anything about the cause of the error. We have limited control over the function that is returning the error. Depending on the native documentation, we may or may not know the state of the resource after receiving the error.

Precisely.

You don't know what state the program is in after getting the error. If you cannot reason about the state of the program, the only reasonable course of action is to crash. Anything else risks undefined behavior which is worse than crashing: it makes things harder to debug and can result in data corruption.

Note this is different from failure modes that are expected and where we can make reasonable assumptions about the state of the program after the error. One example of such a failure mode is failing to open a device: in this case, it is fair to assume that the state of the program is whatever it was before we made the open call (because it would be unreasonable for a native API to be designed any other way), and we can safely recover. That is not true in general when getting an error from a close function.

what happens if you try to close a device that's been unplugged? Does the native API return an error? who knows.

Any reasonable native API should return OK when attempting to close an unplugged device. An unplugged device is not sufficient justification for the handle to enter some kind of "wedged" state where it can't be closed - that would be a native API bug.

Alternatively, it's possible that a native API may be designed to return an error from close, but the handle is in fact closed. (Ideally the native API would document this.) In this case, and only in this case, we can keep going, because we know the state of the program after the call and it is recoverable (i.e. the handle is not "wedged"). In this case, there should be an if statement that checks for the specific error code that we know actually means "OK" and special case that. We should not keep going in the general case.

It might be indicative of a bug. It could be that a non-bug aspect of the native error model caused an error to be returned. Even if it is indicative of a bug, the bug could be in PortAudio, or it could be somewhere in the native audio stack, down to the driver.

If the bug is in the native audio stack, or in the driver, you still want to crash. You don't know what the consequences of that bug may be - for all you know, the process state may be corrupted. It is not safe to keep going in this situation. It's basically undefined behavior - invariants have been violated.