search

project-d-p / Client

0 stars 0 forks source link

Thread Safety Issue: Segfault due to concurrent access #37

Closed dkglee closed 2 months ago

dkglee commented 2 months ago

Issue - 문제 설명

const FString PlayerId = Player_State->GetPlayerName();
if (!FDataHub::gunfireData.Contains(PlayerId))
{
    return;
}
// 여기서 해당 키가 존재 하지 않다고 에러가 발생함.
gunfire_ = FDataHub::gunfireData[PlayerId];

위의 코드에서 Contains에서 true를 반환하여 이후에 [] 연산자로 값을 조회하였지만 해당 키가 존재하지 않는다는 에러가 발생하게 됨.

현재의 로직

클라이언트에서 서버로부터 받은 정보를 사용하는 로직

-FDataHub의 gunfireData의 경우, 멀티스레드로 Thead A가 쓰는 작업을 담당하고 현재 문제가 발생하는 부분에서 Thread B는 읽는 작업만을 진행함.

예상되는 원인

해결 방안.

이를 해결하기 위해서 다음의 세 가지 방안을 고려했다.

첫 번째 - 읽고 쓸 때, lock 시스템을 제공할 것.

단점

두 번째 - 읽는 구조를 서버와 동일하게 가져가기

단점

하지만 여전히 스위칭하는데 lock을 사용하기 때문에 성능상 문제가 발생할 수 있다. 서버의 경우, 각각의 데이터를 처리함에 있어서 데이터의 중요도가 높기 때문에, 데이터 무결성을 확실하게 막을 필요가 있지만, 클라이언트의 경우에는 아니다.

여담

alignas(64)를 붙이게 되면 더욱 더 큰 효과를 얻을 수 있다. 이는 현대의 캐시 라인의 크기가 대부분 64바이트이기 때문에 해당 읽기 및 쓰기 전용 버퍼의 시작 위치를 64 바이트 단위로 정렬을 하게 될 경우, 캐시 일관성 문제를 피할 수 있다점

세 번째 - Contains 함수를 사용하지 않고 Find 함수 사용하기

const FString PlayerId = Player_State->GetPlayerName();
if (!FDataHub::gunfireData.Contains(PlayerId))
{
    return;
}
// 여기서 해당 키가 존재 하지 않다고 에러가 발생함.
gunfire_ = FDataHub::gunfireData[PlayerId];

즉, 처음 Contains 함수의 경우 해당 키에 맞는 데이터가 존재하는 경우 단순 true 혹은 false를 반환하게 된다.

이 경우, 일시적으로는 해당 키가 존재해 true를 반환하지만, gunfireData[PlayerId]를 진행하는 찰나의 시간에 다른 스레드에서 일시적으로 키를 비활성화 할 수 있게 된다.

그렇다면 [] 연산자는 다음의 함수를 호출하도록 오버로딩 되어 있다.

FORCEINLINE ValueType& FindChecked(KeyConstPointerType Key)
{
    auto* Pair = Pairs.Find(Key);
    check(Pair != nullptr);
    return Pair->Value;
}

이제껏 프로그램이 터지게 된 이유가 check(Pair != nullptr); 메크로에서 터지게 되는 것이다. 이는 이전의 Pairs.Find(Key)를 실행하기 전에 다른 스레드에서 해당 키에 대한 정보가 수정되었기 때문이다.

하지만 Contains가 아니라 애초에 Find 함수를 사용하게 되면 이는 바로 해당 값을 참조하는 포인터를 반환하게 된다.

그리고 두 번 Find를 호출하게 되는 이전의 로직보다 훨씬 더 빠르며 (지금의 로직은 Find 한번으로 해결 가능), 중간에 해당 값에 동시에 접근해도 문제가 없고, 데이터 무결성을 지키지 못하더라도 어차피 데이터는 빠르게 변경되며 당장의 업데이트 된 데이터가 그렇게까지 중요하지 않다.

FORCEINLINE ValueType* Find(KeyConstPointerType Key)
{
    if (auto* Pair = Pairs.Find(Key))
    {
        return &Pair->Value;
    }
    return nullptr;
}

그렇기 때문에 Find를 사용하는 것이 가장 빨라 보인다.

단점 및 여전한 문제점

하지만 만약 Thread A에서 값을 넣는 과정이 new를 통해서 동적할당으로 넣어주게 되는 것이고 이전의 값을 해제하는 형식이라면 이것은 커다란 문제가 되기는 한다.

왜냐하면 참조하는 객체가 중간에 파괴되는 경우가 발생하기 때문이다.

결론

우선은 Find 함수를 통해서 위의 문제를 해결하는 방향으로 결정을 하였다.

데이터를 처리하는 부분에 있어서 해당 데이터의 중요도가 그렇게 높지 않기 때문에, lock으로 인한 오버헤드를 가장 피할 수 있는 Find를 사용하는 것이 가장 최적의 판단이라고 들게 되었다.

데이터 일관성의 완벽한 유지 보다는 시스템의 전반적인 성능과 리소스 관리를 우선하는 것이 더 합리적이라 판단했다.

하지만 위에서 작성한 Find의 문제점이 발생하게 될 여지가 있다면 언제든지 구조를 바꿀 것이다.

dkglee commented 2 months ago

추가 내용

template <typename InitKeyType = KeyType, typename InitValueType = ValueType>
ValueType& Emplace(InitKeyType&& InKey, InitValueType&& InValue)
{
    const FSetElementId PairId = Pairs.Emplace(TPairInitializer<InitKeyType&&, InitValueType&&>(Forward<InitKeyType>(InKey),     
Forward<InitValueType>(InValue)));

    return Pairs[PairId].Value;
}

FSetElementId Emplace(ArgsType&& Args, bool* bIsAlreadyInSetPtr = nullptr)
    {
        // Create a new element.
        FSparseArrayAllocationInfo ElementAllocation = Elements.AddUninitialized();
        SetElementType& Element = *new (ElementAllocation) SetElementType(Forward<ArgsType>(Args));

        SizeType NewHashIndex = ElementAllocation.Index;

        uint32 KeyHash = KeyFuncs::GetKeyHash(KeyFuncs::GetSetKey(Element.Value));
        if (!TryReplaceExisting(KeyHash, Element, NewHashIndex, bIsAlreadyInSetPtr))
        {
            RehashOrLink(KeyHash, Element, NewHashIndex);
        }
        return FSetElementId(NewHashIndex);
    }

bool TryReplaceExisting(uint32 KeyHash, SetElementType& Element, SizeType& InOutElementIndex, bool* bIsAlreadyInSetPtr)
    {
        bool bIsAlreadyInSet = false;
        if (!KeyFuncs::bAllowDuplicateKeys)
        {
            // If the set doesn't allow duplicate keys, check for an existing element with the same key as the element being added.

            // Don't bother searching for a duplicate if this is the first element we're adding
            if (Elements.Num() != 1)
            {
                SizeType ExistingIndex = FindIndexByHash(KeyHash, KeyFuncs::GetSetKey(Element.Value));
                bIsAlreadyInSet = ExistingIndex != INDEX_NONE;
                if (bIsAlreadyInSet)
                {
                    // If there's an existing element with the same key as the new element, replace the existing element with the new element.
                    MoveByRelocate(Elements[ExistingIndex].Value, Element.Value);

                    // Then remove the new element.
                    Elements.RemoveAtUninitialized(InOutElementIndex);

                    // Then point the return value at the replaced element.
                    InOutElementIndex = ExistingIndex;
                }
            }
        }
        if (bIsAlreadyInSetPtr)
        {
            *bIsAlreadyInSetPtr = bIsAlreadyInSet;
        }
        return bIsAlreadyInSet;
}

template<typename T>
FORCEINLINE void MoveByRelocate(T& A, T& B)
{
    // Destruct the previous value of A.
    A.~T();

    // Relocate B into the 'hole' left by the destruction of A, leaving a hole in B instead.
    RelocateConstructItems<T>(&A, &B, 1);
}

template <typename DestinationElementType, typename SourceElementType, typename SizeType>
FORCEINLINE void RelocateConstructItems(void* Dest, const SourceElementType* Source, SizeType Count)
{
    if constexpr (UE::Core::Private::MemoryOps::TCanBitwiseRelocate<DestinationElementType, SourceElementType>::Value)
    {
        /* All existing UE containers seem to assume trivial relocatability (i.e. memcpy'able) of their members,
         * so we're going to assume that this is safe here.  However, it's not generally possible to assume this
         * in general as objects which contain pointers/references to themselves are not safe to be trivially
         * relocated.
         *
         * However, it is not yet possible to automatically infer this at compile time, so we can't enable
         * different (i.e. safer) implementations anyway. */

        FMemory::Memmove(Dest, Source, sizeof(SourceElementType) * Count);
    }
    else
    {
        while (Count)
        {
            // We need a typedef here because VC won't compile the destructor call below if SourceElementType itself has a member called SourceElementType
            typedef SourceElementType RelocateConstructItemsElementTypeTypedef;

            new (Dest) DestinationElementType(*Source);
            ++(DestinationElementType*&)Dest;
            (Source++)->RelocateConstructItemsElementTypeTypedef::~RelocateConstructItemsElementTypeTypedef();
            --Count;
        }
    }
}

즉, 로직 순서는 다음과 같다.

  1. Element를 새로 하나 만듬.
  2. Element에 값을 새팅함.
  3. 이전에 해당 키로 할당된 Element가 있는지 확인함.
  4. 있으면 이전 값의 소멸자를 호출함
  5. 그리고 새로운 값으로 Memmove(memcpy와 같은 역할) 함
  6. 여기서 Dest는 이전 값을 담고 있던 Element를 의미합니다.
  7. 이후 새로운 값을 참조하고 있는 Element를 해제한다.

즉, 한번 생긴 키에 대한 부분은 키를 제거하지 않은 이상 해당 포인터 변수는 항상 일정하다.

즉, 해당 위치에 이전에 담고 있던 객체의 소멸자를 호출하여 객체를 해제하고 새로운 객체를 단순히 메모리를 이동함으로써 해결하는 것을 볼 수 있다.

현재의 로직(우리의 게임의 경우)에는 TMap이 포함하고 있는 객체가 내부적으로 동적할당한 값이 없고, string의 경우에도 Protobuf가 내부적으로 void*로 관리를 하고 있기 때문에 포인터 변수의 크기는 8바이트로 "불변"하다.

그 말은 Find로 했을 때 터질 걱정이 없다.

ps

제가 아는 지식 선에서는 당장의 문제가 발생할 여지는 없어 보입니다. 하지만 언제든지 해당 문제가 발생하거나 틀린 부분이 있다면 comment를 남겨주세요!

감사합니다!