Skip to content

Commit

Permalink
Make the context error state thread-local
Browse files Browse the repository at this point in the history
This is very much not specified by the standard, but it is necessary to allow
thread-safe usage and error checking. Otherwise, if two threads are making AL
calls (say, one thread handling a streaming buffer queue and another to play
and update sound sources) and checking for errors, it's possible for one thread
to generate an error that the other thread reads, causing the one thread to not
see the error and think its calls were okay despite the failure, and the other
thread to think its calls caused an error when it was actually successful. This
is a possible, if not likely, scenario in existing apps.

This probably violates the current standard, however I can't think of a useful
scenario where this would be a significant problem. It would essentially rely
on the app knowingly reading errors from a separate thread than the one that
generated them, which would be a problem for actually reacting to the failed
call or having useful information to log the error with. That's if the app is
even generating errors to begin with.

If this does prove to be a problem, a compat option can be added to restore the
original behavior. Or in the worst case, revert to the original behavior by
default and have an opt-in flag for apps that want thread-local error state.
But for now, let's assume existing apps will be fine, if not fix potention race
conditions.
  • Loading branch information
kcat committed Feb 13, 2024
1 parent 50c906f commit 0584aec
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 4 deletions.
1 change: 1 addition & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,7 @@ set(COMMON_OBJS
common/alstring.h
common/althrd_setname.cpp
common/althrd_setname.h
common/althreads.h
common/altraits.h
common/atomic.h
common/comptr.h
Expand Down
9 changes: 6 additions & 3 deletions al/error.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,8 @@ void ALCcontext::setError(ALenum errorCode, const char *msg, ...)
#endif
}

ALenum curerr{AL_NO_ERROR};
mLastError.compare_exchange_strong(curerr, errorCode);
if(mLastThreadError.get() == AL_NO_ERROR)
mLastThreadError.set(errorCode);

debugMessage(DebugSource::API, DebugType::Error, 0, DebugSeverity::High,
{msg, static_cast<uint>(msglen)});
Expand Down Expand Up @@ -137,5 +137,8 @@ AL_API auto AL_APIENTRY alGetError() noexcept -> ALenum

FORCE_ALIGN ALenum AL_APIENTRY alGetErrorDirect(ALCcontext *context) noexcept
{
return context->mLastError.exchange(AL_NO_ERROR);
ALenum ret{context->mLastThreadError.get()};
if(ret != AL_NO_ERROR) UNLIKELY
context->mLastThreadError.set(AL_NO_ERROR);
return ret;
}
3 changes: 2 additions & 1 deletion alc/context.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "al/listener.h"
#include "almalloc.h"
#include "alnumeric.h"
#include "althreads.h"
#include "atomic.h"
#include "core/context.h"
#include "inprogext.h"
Expand Down Expand Up @@ -80,7 +81,7 @@ struct ALCcontext : public al::intrusive_ref<ALCcontext>, ContextBase {

std::mutex mPropLock;

std::atomic<ALenum> mLastError{AL_NO_ERROR};
al::tss<ALenum> mLastThreadError{AL_NO_ERROR};

const ContextFlagBitset mContextFlags;
std::atomic<bool> mDebugEnabled{false};
Expand Down
125 changes: 125 additions & 0 deletions common/althreads.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
#ifndef AL_THREADS_H
#define AL_THREADS_H

#include <cstdint>
#include <stdexcept>
#include <type_traits>

#ifdef _WIN32
#define WIN32_LEAN_AND_MEAN
#include <windows.h>

#else

#include <threads.h>
#endif

#include "albit.h"

namespace al {

template<typename T>
class tss {
static_assert(sizeof(T) <= sizeof(void*));
static_assert(std::is_trivially_destructible_v<T> && std::is_trivially_copy_constructible_v<T>);

static void *as_ptr(const T &value) noexcept
{
if constexpr(std::is_pointer_v<T>)
{
if constexpr(std::is_const_v<std::remove_pointer_t<T>>)
return const_cast<void*>(static_cast<const void*>(value)); /* NOLINT(*-const-cast) */
else
return static_cast<void*>(value);
}
else if constexpr(sizeof(T) == sizeof(void*))
return al::bit_cast<void*>(value);
else if constexpr(std::is_integral_v<T>)
return al::bit_cast<void*>(static_cast<std::uintptr_t>(value));
}

#ifdef _WIN32
DWORD mTss;

public:
tss()
{
mTss = TlsAlloc();
if(mTss == TLS_OUT_OF_INDEXES)
throw std::runtime_error{"al::tss::tss()"};
}
explicit tss(const T &init) : tss{}
{
if(TlsSetValue(mTss, as_ptr(init)) == FALSE)
throw std::runtime_error{"al::tss::tss(T)"};
}
tss(const tss&) = delete;
tss(tss&&) = delete;
~tss() { TlsFree(mTss); }

void operator=(const tss&) = delete;
void operator=(tss&&) = delete;

void set(const T &value)
{
if(TlsSetValue(mTss, as_ptr(value)) == FALSE)
throw std::runtime_error{"al::tss::set(T)"};
}

[[nodiscard]]
auto get() const noexcept -> T
{
void *res{TlsGetValue(mTss)};
if constexpr(std::is_pointer_v<T>)
return static_cast<T>(res);
else if constexpr(sizeof(T) == sizeof(void*))
return al::bit_cast<T>(res);
else if constexpr(std::is_integral_v<T>)
return static_cast<T>(al::bit_cast<std::uintptr_t>(res));
}

#else

tss_t mTss;

public:
tss()
{
if(int res{tss_create(&mTss, nullptr)}; res != thrd_success)
throw std::runtime_error{"al::tss::tss()"};
}
explicit tss(const T &init) : tss{}
{
if(int res{tss_set(mTss, as_ptr(init))}; res != thrd_success)
throw std::runtime_error{"al::tss::tss(T)"};
}
tss(const tss&) = delete;
tss(tss&&) = delete;
~tss() { tss_delete(mTss); }

void operator=(const tss&) = delete;
void operator=(tss&&) = delete;

void set(const T &value)
{
if(int res{tss_set(mTss, as_ptr(value))}; res != thrd_success)
throw std::runtime_error{"al::tss::set(T)"};
}

[[nodiscard]]
auto get() const noexcept -> T
{
void *res{tss_get(mTss)};
if constexpr(std::is_pointer_v<T>)
return static_cast<T>(res);
else if constexpr(sizeof(T) == sizeof(void*))
return al::bit_cast<T>(res);
else if constexpr(std::is_integral_v<T>)
return static_cast<T>(al::bit_cast<std::uintptr_t>(res));
}
#endif /* _WIN32 */
};

} // namespace al

#endif /* AL_THREADS_H */

0 comments on commit 0584aec

Please sign in to comment.