Crash "NotCoreThead"

[KirmesBude]

[FATAL] A fatal error occurred and the program has to terminate!
  - Error: InternalErrorException
  - Description: This method can only be accessed from the core thread.
  - In function: void bs::throwIfNotCoreThread()
  - In file: /home/bude/myGit/REGoth-bs/lib/bsf/Source/Foundation/bsfCore/CoreThread/BsCoreThread.cpp:384

Stack trace: 
0) lib/libbsf.so.1: bs::CrashHandler::getStackTrace[abi:cxx11]()+0x1de [0x7f3d1a3573fe]
1) lib/libbsf.so.1: bs::CrashHandler::logErrorAndStackTrace(std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, unsigned int) const+0x354 [0x7f3d1a334444]
2) lib/libbsf.so.1: bs::CrashHandler::reportCrash(std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, bs::StdAlloc<char, bs::GenAlloc> > const&, unsigned int) const+0x5f [0x7f3d1a357d2f]
3) lib/libbsf.so.1: bs::throwIfNotCoreThread()+0x22a [0x7f3d1a37a77a]
4) lib/libbsf.so.1: bs::ct::CoreObject::~CoreObject()+0x17 [0x7f3d1a37ebd7]
5) lib/libbsf.so.1: std::_Sp_counted_deleter<bs::ct::Material*, bs::Deleter<bs::ct::Material, bs::GenAlloc>, bs::StdAlloc<bs::ct::Material, bs::GenAlloc>, (__gnu_cxx::_Lock_policy)2>::_M_dispose()+0x11c [0x7f3d1a56da5c]
6) lib/libbsf.so.1: bs::CoreObjectManager::syncDownload(bs::FrameAlloc*)+0x5d2 [0x7f3d1a3761d2]
7) lib/libbsf.so.1: bs::CoreObjectManager::syncToCore()+0x31 [0x7f3d1a376321]
8) lib/libbsfRenderBeast.so: bs::ct::RenderBeast::renderAll(bs::PerFrameData)+0x70 [0x7f3d0cebbd00]
9) lib/libbsf.so.1: bs::CoreApplication::runMainLoopFrame()+0x3db [0x7f3d1a6ebb7b]
10) lib/libbsf.so.1: bs::CoreApplication::runMainLoop()+0x50 [0x7f3d1a6ed730]
11) ./bin/REGoth: REGoth::Engine::run()+0xdf [0x55dc64aef8cf]
12) ./bin/REGoth: REGoth::runEngine(REGoth::Engine&)+0x520 [0x55dc64afb930]
13) ./bin/REGoth: main+0x1e0 [0x55dc64ac4090]
14) /lib/x86_64-linux-gnu/libc.so.6: __libc_start_main+0xe7 [0x7f3d19108b97]
15) ./bin/REGoth: _start+0x2a [0x55dc64acff6a]

Seems to have been introduced a while back, but I can not say for sure when.
I am also not really sure why this happens or how to fix. I guess we are doing something "low level" now somewhere which should be in the CoreThread?
But how did this not get immediately found?

[ataulien]

I assume this was introduced when I added multithreaded bulk-caching in commit 468360f.

If you comment out the line here, does it still crash?

REGoth-bs/src/original-content/OriginalGameResources.cpp

Lines 14 to 17 in 468360f

	void OriginalGameResources::populateCache()
	{
	BsZenLib::CacheWholeVDFS(gVirtualFileSystem().getFileIndex());
	}

It might worked for me be because I'm using the "low-fidelity" render-pipeline of bsf...

[KirmesBude]

EDIT: it is 74b53b0.
EDIT2: Looks like BsMaterial might have some dependency on running on the Core Thread
EDIT3: Yeah, I dont know. I guess for some CoreObject deconstructor is called, but I dont see where that happens for us.
On another note, I have a problem in the previous commits where I only see some black texture :D
No, that does not appear to be it.
I will try to find out which commit causes this, probably easier to go from there.

[KirmesBude]

By @mworchel in gitter:

void VisualSkeletalAnimation::destroyClonedMaterials()
{
  for (bs::HMaterial m : mClonedMaterials)
  {
    m->destroy();
  }
  mClonedMaterials.clear();
}

The m->destroy(); is the cause for the crash
Don't know why but it seems that the material core object is not deleted on the core thread. I would just skip the explicit destroy call and just clear the list of cloned materials. bs::HMaterial does reference counting anyway and if the material is not referenced anymore, it should be deleted...I hope

[silviu-life]

Hi, thanks for the pointer, I've been trying to make this work on Friday.

To me, the VisualSkeletalAnimation::destroyClonedMaterials() method seems a bit weird using a member field. It seems to work for me with a bit of refactoring:

  void VisualSkeletalAnimation::cloneMaterials()
  {
    // Access the original materials for cloning
    const auto& materials = mMesh->getMaterials();

    bs::Vector<bs::HMaterial> clonedMaterials(materials.size());
    for (bs::UINT32 i = 0; i < (bs::UINT32)clonedMaterials.size(); i++)
    {
      clonedMaterials[i] = materials[i]->clone();
    }

    mSubRenderable->setMaterials(clonedMaterials);
  }

NOTE: I don't know the code or the libraries it's using very well, but I don't see why materials need to be destroyed, since this should be done by HRenderable, and I also don't see why they need to be memorized in VisualSkeletalAnimation

[ataulien]

I'm not sure whether resources are destroyed when they're not used anymore. If the visual changes meshes, materials would have to be cloned again. If we don't get the clones from the previous mesh deleted, we would have made us a memory leak.

If the cloned materials are really destroyed because of ref-counting, then destroying them manually is certainly wrong.

[mworchel]

HMaterial is a typedef of ResourceHandle<Material> which is in turn a typedef for TResourceHandle<Material, WeakHandle=false>. This class derives from TResourceHandleBase<false> and calls TResourceHandleBase<false>::releaseRef() in the destructor which is defined as

void releaseRef()
{
	if (mData)
	{
		std::uint32_t refCount = mData->mRefCount.fetch_sub(1, std::memory_order_release);

		if (refCount == 1)
		{
			std::atomic_thread_fence(std::memory_order_acquire);
			destroy();
		}
	}
};

So essentially it also calls destroy only that it has this atomic_thread_fence call which I'm not aware of the details. From the looks, this means no memory leaks should be introduced by PR #114 .

[silviu-life]

Thanks Markus, makes sense, and the destructor probably gets called in this method:

template<bool Core>
void TRenderable<Core>::setMaterials(const Vector<MaterialType>& materials)
{
	UINT32 numMaterials = (UINT32)mMaterials.size();
	UINT32 min = std::min(numMaterials, (UINT32)materials.size());

	for (UINT32 i = 0; i < min; i++)
		mMaterials[i] = materials[i];

	for (UINT32 i = min; i < numMaterials; i++)
		mMaterials[i] = nullptr;

	_markDependenciesDirty();
	_markResourcesDirty();
	_markCoreDirty();
}

More precisely:

for (UINT32 i = min; i < numMaterials; i++)
	mMaterials[i] = nullptr;