Open dmitrykos opened 1 year ago
will solve issues similar to #766
As discussed on that issue, I don't think it will. By the time the code gets to call your proposed API, all bets are off and it's already too late.
On the other hand, your proposal might help with thread safety (#808): we could allow concurrent calls to the PortAudio API as long as different contexts are used. I'm not sure how useful that would be in practice, though.
@dechamps when you have 2 contexts created in 2 different processes they will not cross each other, even if application is using 2 different PA versions because there are no static variables which can be referenced from 2 instances. Therefore they will really solve the issue described in #766 without a need to introduce some static variables as semaphores (which are to my view absolutely unreliable) during the initialization stage.
I'm not sure how useful that would be in practice, though.
Per-thread context is indeed very useful as you can keep context per thread. For example, according to my proposal in #808, only thread which created context will be authorized to call PA thread-unsafe API.
when you have 2 contexts created in 2 different processes they will not cross each other, even if application is using 2 different PA versions because there are no static variables which can be referenced from 2 instances
"Static variables" are only a small part of the problem. Let me take an example:
Let's say you have two PortAudio versions, version 1 and version 2.
PortAudio version 2 exposes a function called Pa_ShinyNewFeature()
. This function does not exist in version 1.
Now, let's say you have an application that loads two modules, module A and module B. Both modules come bundled with their own portaudio.dll
. (This is the exact scenario that played out in #766.) The one bundled with module A is PortAudio version 1, while the one bundled with module B is PortAudio version 2.
Let's say module A is loaded first, resulting in portaudio.dll
version 1 being loaded.
Now module B is loaded. Because there is already a DLL with the name portaudio.dll
loaded in the process, module B's version of portaudio.dll
does not get loaded; instead, the Windows DLL loader links module B to the existing instance of portaudio.dll
, i.e. version 1 from module A.
Now module B tries to call Pa_ShinyNewFeature()
, which doesn't exist. Hilarity ensues. (Actually I don't think it will even get to that point - most likely module B will fail to load entirely because of the incomplete import table.)
A similar problem occurs if module B tries to use parameters (e.g. flags) that PortAudio version 1 does not understand, resulting in undefined/surprising behavior from the perspective of module B.
Your proposed fix will not help with this. The only way to fix this is to either (1) ensure module A or module B use different PortAudio DLL names (e.g. portaudio_a.dll
and portaudio_b.dll
), or (2) adjust module A and/or B to use a side-by-side assembly manifest (example) that instructs the Windows DLL loader to not reuse an existing DLL instance. None of these solutions can be implemented on the PortAudio side; it is up to PortAudio users to set them up.
they will really solve the issue described in https://github.com/PortAudio/portaudio/issues/766 without a need to introduce some static variables as semaphores
To be clear, this "semaphore" static variable you are referring to does not "solve the issue". It only attempts to mitigate the blast radius if this problem occurs by degrading more gracefully (instead of crashing). The only way to "solve the issue" is to use one of the techniques I just described.
Per-thread context is indeed very useful as you can keep context per thread
I agree it might be useful, I'm just not convinced applications would really make use of this in practice. Are there really applications that want to run multiple PortAudio instances from multiple threads? Is that really that useful? I don't have strong feelings about this though. I agree that pretty much anything is better than global state.
A similar problem occurs if module B tries to use parameters (e.g. flags) that PortAudio version 1 does not understand, resulting in undefined/surprising behavior from the perspective of module B.
@dechamps it is easy to fix the problem you described on per context basis: PaContext holds PA API version and pa_front.c
public functions check whether PA API version matches when context is provided as one of the parameters, otherwise call gets rejected with some error code. Therefore you no longer need any static variable to prevent multiple simultaneous calls, it is automatically solved by per-context approach.
Per-context API does not solve ABI compatibility fully though (which you described), therefore to prevent unknown/incompatible ABI user has to check PA version before any other calls to PA API:
if (Pa_GetVersion() != paMakeVersionNumber(19,5,6))
{
// fail
}
A good example of per-context implementation is libusb: https://github.com/libusb/libusb/blob/fcf0c710ef5911ae37fbbf1b39d48a89f6f14e8a/libusb/libusb.h#L1567-L1571
Definitely a breaking change.
Definitely a breaking change.
@RossBencina it can be made non-breaking if context-aware functions bear different names, postfixed with Ctx
(context) or Ex
(extended). Legacy API would stay with its function names as-is without changes and preserving ABI compatibility fully.
PaError Pa_InitializeCtx( PaContext context )
PaError Pa_TerminateCtx( PaContext context )
const PaHostErrorInfo* Pa_GetLastHostErrorInfoCtx( PaContext context )
or
PaError Pa_InitializeEx( PaContext context )
PaError Pa_TerminateEx( PaContext context )
const PaHostErrorInfo* Pa_GetLastHostErrorInfoEx( PaContext context )
Library Context will remove dependency on static variables instantiated in
pa_front.c
and will solve issues similar to #766. It also makes PA API more versatile, for example user would keep Library Context in a thread context which is calling PA API.Context management functions:
Modified API:
PaStream
would holdPaContext
instance, therefore stream managing API, such asPaError Pa_StartStream( PaStream *stream )
will stay unmodified.Context-less API (current) would stay as legacy API and use a global statically allocated instance of
PaContext
to provide backwards compatibility with apps not using Context-aware new API.I can take this task if there is mutual agreement on such enhancement.