mirror of
https://github.com/Relintai/pandemonium_engine.git
synced 2024-12-23 12:26:59 +01:00
Ported: Improve locking safety for RID handles builds. Additional locks are added. This is primarily to cover a potential race condition where the pool is resized from another thread during a get operation. - lawnjelly
130cc36a88
This commit is contained in:
parent
309055ce7f
commit
a2cb12fd07
@ -66,7 +66,7 @@ RID_Database::RID_Database() {
|
|||||||
RID_Database::~RID_Database() {
|
RID_Database::~RID_Database() {
|
||||||
}
|
}
|
||||||
|
|
||||||
String RID_Database::_rid_to_string(const RID &p_rid, const PoolElement &p_pe) {
|
String RID_Database::_rid_to_string(const RID &p_rid, const PoolElement &p_pe) const {
|
||||||
String s = "RID id=" + itos(p_rid._id);
|
String s = "RID id=" + itos(p_rid._id);
|
||||||
s += " [ rev " + itos(p_rid._revision) + " ], ";
|
s += " [ rev " + itos(p_rid._revision) + " ], ";
|
||||||
|
|
||||||
@ -214,7 +214,7 @@ RID_Data *RID_Database::handle_get(const RID &p_rid) {
|
|||||||
return data;
|
return data;
|
||||||
}
|
}
|
||||||
|
|
||||||
RID_Data *RID_Database::handle_getptr(const RID &p_rid) {
|
RID_Data *RID_Database::handle_getptr(const RID &p_rid) const {
|
||||||
RID_Data *data = handle_get_or_null(p_rid);
|
RID_Data *data = handle_get_or_null(p_rid);
|
||||||
#ifdef RID_HANDLE_FLAG_NULL_GETS
|
#ifdef RID_HANDLE_FLAG_NULL_GETS
|
||||||
ERR_FAIL_COND_V_MSG(!data, nullptr, "RID_Database getptr is NULL");
|
ERR_FAIL_COND_V_MSG(!data, nullptr, "RID_Database getptr is NULL");
|
||||||
@ -222,18 +222,12 @@ RID_Data *RID_Database::handle_getptr(const RID &p_rid) {
|
|||||||
return data;
|
return data;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Note, no locks used in the getters.
|
RID_Data *RID_Database::handle_get_or_null(const RID &p_rid) const {
|
||||||
// Pandemonium 4.x does use locks in the getters, but it is arguably overkill because even though
|
|
||||||
// the pointer returned will be correct (i.e. it has not been replaced during this call),
|
|
||||||
// it can be invalidated during the client code use. (There may also be an internal reason why
|
|
||||||
// locks are needed in 4.x, as the database is different.)
|
|
||||||
// An example of a "safer" way to do this kind of thing is object level locking,
|
|
||||||
// (but that has complications of its own), or atomic object changes.
|
|
||||||
|
|
||||||
RID_Data *RID_Database::handle_get_or_null(const RID &p_rid) {
|
|
||||||
if (p_rid.is_valid()) {
|
if (p_rid.is_valid()) {
|
||||||
ERR_FAIL_COND_V_MSG(_shutdown, nullptr, "RID_Database get_or_null after shutdown.");
|
ERR_FAIL_COND_V_MSG(_shutdown, nullptr, "RID_Database get_or_null after shutdown.");
|
||||||
|
|
||||||
|
MutexLock lock(_mutex);
|
||||||
|
|
||||||
// The if statement is to allow breakpointing without a recompile.
|
// The if statement is to allow breakpointing without a recompile.
|
||||||
if (p_rid._id >= _pool.pool_reserved_size()) {
|
if (p_rid._id >= _pool.pool_reserved_size()) {
|
||||||
ERR_FAIL_COND_V_MSG(p_rid._id >= _pool.pool_reserved_size(), nullptr, "RID_Database get_or_null, RID id was outside pool size.");
|
ERR_FAIL_COND_V_MSG(p_rid._id >= _pool.pool_reserved_size(), nullptr, "RID_Database get_or_null, RID id was outside pool size.");
|
||||||
@ -249,47 +243,46 @@ RID_Data *RID_Database::handle_get_or_null(const RID &p_rid) {
|
|||||||
return nullptr;
|
return nullptr;
|
||||||
}
|
}
|
||||||
|
|
||||||
bool RID_Database::handle_owns(const RID &p_rid) const {
|
bool RID_Database::handle_is_owner(const RID &p_rid, const RID_OwnerBase *p_owner) const {
|
||||||
ERR_FAIL_COND_V_MSG(_shutdown, false, "RID_Database owns after shutdown.");
|
if (p_rid.is_valid()) {
|
||||||
|
ERR_FAIL_COND_V_MSG(_shutdown, false, "RID_Database handle_is_owner after shutdown.");
|
||||||
|
|
||||||
if (!p_rid.is_valid()) {
|
MutexLock lock(_mutex);
|
||||||
return false;
|
|
||||||
|
// The if statement is to allow breakpointing without a recompile.
|
||||||
|
if (p_rid._id >= _pool.pool_reserved_size()) {
|
||||||
|
ERR_FAIL_COND_V_MSG(p_rid._id >= _pool.pool_reserved_size(), false, "RID_Database handle_is_owner, RID id was outside pool size.");
|
||||||
|
}
|
||||||
|
|
||||||
|
const PoolElement &pe = _pool[p_rid._id];
|
||||||
|
if (pe.revision != p_rid._revision) {
|
||||||
|
ERR_FAIL_COND_V_MSG(pe.revision != p_rid._revision, false, "RID handle_is_owner, revision is incorrect, possible dangling RID. " + _rid_to_string(p_rid, pe));
|
||||||
|
}
|
||||||
|
|
||||||
|
return pe.data->_owner == p_owner;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (p_rid._id >= _pool.pool_reserved_size()) {
|
return false;
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
const PoolElement &pe = _pool[p_rid._id];
|
|
||||||
if (pe.revision != p_rid._revision) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
if (!pe.data) {
|
|
||||||
return false;
|
|
||||||
}
|
|
||||||
|
|
||||||
return true;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
void RID_Database::handle_free(const RID &p_rid) {
|
void RID_Database::handle_free(const RID &p_rid) {
|
||||||
ERR_FAIL_COND_MSG(_shutdown, "RID_Database free after shutdown.");
|
ERR_FAIL_COND_MSG(_shutdown, "RID_Database free after shutdown.");
|
||||||
bool revision_correct = true;
|
bool revision_correct = true;
|
||||||
|
|
||||||
ERR_FAIL_COND_MSG(p_rid._id >= _pool.pool_reserved_size(), "RID_Database free, RID id was outside pool size.");
|
{
|
||||||
_mutex.lock();
|
MutexLock lock(_mutex);
|
||||||
PoolElement &pe = _pool[p_rid._id];
|
ERR_FAIL_COND_MSG(p_rid._id >= _pool.pool_reserved_size(), "RID_Database free, RID id was outside pool size.");
|
||||||
revision_correct = pe.revision == p_rid._revision;
|
PoolElement &pe = _pool[p_rid._id];
|
||||||
|
revision_correct = pe.revision == p_rid._revision;
|
||||||
|
|
||||||
// mark the data as zero, which indicates unused element
|
// mark the data as zero, which indicates unused element
|
||||||
if (revision_correct) {
|
if (revision_correct) {
|
||||||
pe.data->_owner = nullptr;
|
pe.data->_owner = nullptr;
|
||||||
pe.data = nullptr;
|
pe.data = nullptr;
|
||||||
_pool.free(p_rid._id);
|
_pool.free(p_rid._id);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
_mutex.unlock();
|
|
||||||
|
|
||||||
ERR_FAIL_COND_MSG(!revision_correct, "RID_Database free, revision is incorrect, object possibly freed more than once.");
|
ERR_FAIL_COND_MSG(!revision_correct, "RID_Database free, revision is incorrect, object possibly freed more than once.");
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -299,10 +292,12 @@ RID RID_Database::prime(const RID &p_rid, int p_line_number, const char *p_filen
|
|||||||
ERR_FAIL_COND_V_MSG(_shutdown, p_rid, "RID_Database prime after shutdown.");
|
ERR_FAIL_COND_V_MSG(_shutdown, p_rid, "RID_Database prime after shutdown.");
|
||||||
ERR_FAIL_COND_V_MSG(p_rid._id >= _pool.pool_reserved_size(), p_rid, "RID_Database prime, RID id was outside pool size.");
|
ERR_FAIL_COND_V_MSG(p_rid._id >= _pool.pool_reserved_size(), p_rid, "RID_Database prime, RID id was outside pool size.");
|
||||||
|
|
||||||
|
// We could also probably get away without using a lock here if purely for debugging, and not for release editor / templates.
|
||||||
|
MutexLock lock(_mutex);
|
||||||
|
|
||||||
PoolElement &pe = _pool[p_rid._id];
|
PoolElement &pe = _pool[p_rid._id];
|
||||||
ERR_FAIL_COND_V_MSG(pe.revision != p_rid._revision, p_rid, "RID_Database prime, revision is incorrect, object possibly freed before use.");
|
ERR_FAIL_COND_V_MSG(pe.revision != p_rid._revision, p_rid, "RID_Database prime, revision is incorrect, object possibly freed before use.");
|
||||||
|
|
||||||
// no threading checks as it the tracking info doesn't matter if there is a race condition
|
|
||||||
pe.line_number = p_line_number;
|
pe.line_number = p_line_number;
|
||||||
pe.filename = p_filename;
|
pe.filename = p_filename;
|
||||||
}
|
}
|
||||||
|
@ -141,7 +141,7 @@ class RID_Database {
|
|||||||
// is treated as a POD type.
|
// is treated as a POD type.
|
||||||
TrackedPooledList<PoolElement, uint32_t, true, true> _pool;
|
TrackedPooledList<PoolElement, uint32_t, true, true> _pool;
|
||||||
bool _shutdown = false;
|
bool _shutdown = false;
|
||||||
Mutex _mutex;
|
mutable Mutex _mutex;
|
||||||
|
|
||||||
// This is purely for printing the leaks at the end, as RID_Owners may be
|
// This is purely for printing the leaks at the end, as RID_Owners may be
|
||||||
// destroyed before the RID_Database is shutdown, so the RID_Data may be invalid
|
// destroyed before the RID_Database is shutdown, so the RID_Data may be invalid
|
||||||
@ -152,7 +152,7 @@ class RID_Database {
|
|||||||
LocalVector<Leak> _leaks;
|
LocalVector<Leak> _leaks;
|
||||||
|
|
||||||
void register_leak(uint32_t p_line_number, uint32_t p_owner_name_id, const char *p_filename);
|
void register_leak(uint32_t p_line_number, uint32_t p_owner_name_id, const char *p_filename);
|
||||||
String _rid_to_string(const RID &p_rid, const PoolElement &p_pe);
|
String _rid_to_string(const RID &p_rid, const PoolElement &p_pe) const;
|
||||||
|
|
||||||
public:
|
public:
|
||||||
RID_Database();
|
RID_Database();
|
||||||
@ -168,10 +168,11 @@ public:
|
|||||||
RID prime(const RID &p_rid, int p_line_number, const char *p_filename);
|
RID prime(const RID &p_rid, int p_line_number, const char *p_filename);
|
||||||
|
|
||||||
void handle_make_rid(RID &r_rid, RID_Data *p_data, RID_OwnerBase *p_owner);
|
void handle_make_rid(RID &r_rid, RID_Data *p_data, RID_OwnerBase *p_owner);
|
||||||
RID_Data *handle_get(const RID &p_rid);
|
RID_Data *handle_get(const RID &p_rid) const;
|
||||||
RID_Data *handle_getptr(const RID &p_rid);
|
RID_Data *handle_getptr(const RID &p_rid) const;
|
||||||
RID_Data *handle_get_or_null(const RID &p_rid);
|
RID_Data *handle_get_or_null(const RID &p_rid) const;
|
||||||
bool handle_owns(const RID &p_rid) const;
|
|
||||||
|
bool handle_is_owner(const RID &p_rid, const RID_OwnerBase *p_owner) const;
|
||||||
void handle_free(const RID &p_rid);
|
void handle_free(const RID &p_rid);
|
||||||
};
|
};
|
||||||
|
|
||||||
@ -180,15 +181,7 @@ extern RID_Database g_rid_database;
|
|||||||
class RID_OwnerBase {
|
class RID_OwnerBase {
|
||||||
protected:
|
protected:
|
||||||
bool _is_owner(const RID &p_rid) const {
|
bool _is_owner(const RID &p_rid) const {
|
||||||
const RID_Data *p = g_rid_database.handle_get_or_null(p_rid);
|
return g_rid_database.handle_is_owner(p_rid, this);
|
||||||
return (p && (p->_owner == this));
|
|
||||||
}
|
|
||||||
|
|
||||||
void _remove_owner(RID &p_rid) {
|
|
||||||
RID_Data *p = g_rid_database.handle_get_or_null(p_rid);
|
|
||||||
if (p) {
|
|
||||||
p->_owner = nullptr;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
void _rid_print(const char *pszType, String sz, const RID &p_rid);
|
void _rid_print(const char *pszType, String sz, const RID &p_rid);
|
||||||
@ -237,7 +230,6 @@ public:
|
|||||||
#ifdef RID_HANDLE_PRINT_LIFETIMES
|
#ifdef RID_HANDLE_PRINT_LIFETIMES
|
||||||
_rid_print(_typename, "free_rid", p_rid);
|
_rid_print(_typename, "free_rid", p_rid);
|
||||||
#endif
|
#endif
|
||||||
_remove_owner(p_rid);
|
|
||||||
g_rid_database.handle_free(p_rid);
|
g_rid_database.handle_free(p_rid);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user