Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

If you create a NovelRT::Ecs::SystemScheduler as a variable which is then later initialised, this causes a crash #596

Open
dndn1011 opened this issue Nov 23, 2023 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@dndn1011
Copy link

Note: for support questions, please use the #engine-user-help channel in our Discord or create a discussion. This repository's issues are reserved for feature requests and bug reports.

Describe the issue:
If you create a NovelRT::Ecs::SystemScheduler as a variable which is then later initialised, this generates no compile error or warnings, but the engine will fail due to conflicts caused by thread shutdown in the move function.

    SystemScheduler::SystemScheduler(SystemScheduler&& other) noexcept
        : _systemIds(std::move(other._systemIds)),
          _typedSystemCache(std::move(other._typedSystemCache)),
          _systems(std::move(other._systems)),
          _entityCache(std::move(other._entityCache)),
          _componentCache(std::move(other._componentCache)),
          _workerThreadCount(other._workerThreadCount),
          _currentDelta(other._currentDelta),
          _threadAvailabilityMap((1ULL << other._workerThreadCount) - 1),
          _shouldShutDown(false),
          _threadsAreSpinning(false)
    {
        if (other.GetThreadsAreSpinning())
        {
            other.ShutDown();
            SpinThreads();
        }
    }

I suspect it should not be shutting down the threads when the SystemScheduler is moved.

Please provide the steps to reproduce if possible:

  1. Clone the repo
  2. Go to a sample
  3. Instead of initialising the scheduler directy with an initialiser, define the scheduler first and the assign to it:
auto scheduler =
    Configurator()
    ...

||
V

NovelRT::Ecs::SystemScheduler scheduler;
scheduler =
    Configurator()
    ...
  1. Build and run
  2. Observe crash

Expected behaviour:
It should not crash in this case, or this use of SystemScheduler should be disabled.

Please tell us about your environment:
N/A

Additional context:
I did attempt to remove the threads shut down in the move function, but this just resulted in a crash in ucrtbased with no useful stack information. I do wonder about there being two SystemScheduler s in existence briefly. One is constructed with its default constructor, and the second with arguments - I have a feeling that this is not intended behaviour or that if intended there is some contention between instances of system resources. I did try deleting the default constructor, but this caused compile issues elsewhere in the code.

I am working around this for now by simply putting SystemScheduler in a unique_ptr and initialising in one go with make_unique

@dndn1011 dndn1011 added the bug Something isn't working label Nov 23, 2023
@RubyNova
Copy link
Member

What's most likely happening is that its trying to stop threads from spinning that never started. When the scheduler moves it has to terminate any spinning threads because std::thread objects cannot be moved from what I recall, which creates a whole hassle of issues.

I'll look into it later today.

@RubyNova RubyNova self-assigned this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants