stride icon indicating copy to clipboard operation
stride copied to clipboard

feat: Load Services only on first call

Open Doprez opened this issue 7 months ago • 2 comments

PR Details

This PR is to allow systems to be modified from the Game class and not set services until it is first called.

Related Issue

https://github.com/stride3d/stride/issues/2628 Semi related https://github.com/stride3d/stride/pull/2806 This will be useful when avoiding injecting systems at startup.

Types of changes

  • [ ] Docs change / refactoring / dependency upgrade
  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • [ ] My change requires a change to the documentation.
  • [ ] I have added tests to cover my changes.
  • [ ] All new and existing tests passed.
  • [ ] I have built and run the editor to try this change out.

Doprez avatar Jun 01 '25 17:06 Doprez

Are calls to Services.GetSafeServiceAs<T>()` thread-safe? If not, two threads initializing for the first time could get different instances.

Kryptos-FR avatar Jun 01 '25 19:06 Kryptos-FR

~~None of the service registry calls are thread safe AFAICT~~. It is locked on call.

    public T? GetService<T>()
        where T : class
    {
        var type = typeof(T);
        lock (registeredService)
        {
            if (registeredService.TryGetValue(type, out var service))
                return (T)service;
        }

        return null;
    }

Although that does bring up a great point and the threaded instance references issues could be something to consider here.

GetSafeServiceAs is just meant to throw when the service is null instead of just returning the null, non-existent reference.

    public static T GetSafeServiceAs<T>(this IServiceRegistry registry)
        where T : class
    {
        return registry.GetService<T>() ?? throw new ServiceNotFoundException(typeof(T));
    }

Doprez avatar Jun 01 '25 19:06 Doprez

GetService does not create the service, and since calls to that are thread safe as Doprez mentioned, the only way for a user to get two different instances of a given service type would be if they are removing the service from a different thread while accessing it from the main thread. I think it's fair to leave this edge case as is. The user is clearly playing with fire here. Is this ready to be merged ? What's up with the Streaming and Content service being omitted ?

Eideren avatar Aug 27 '25 15:08 Eideren

What's up with the Streaming and Content service being omitted ?

No real reason other than they are very core concepts so I must have thought they werent needed at the time. I can add the same functionality for consistency though.

Is this ready to be merged ?

Ill add those 2 today and it can be merged. Might as well make it a bit more modular.

Doprez avatar Aug 27 '25 15:08 Doprez

All done

Doprez avatar Aug 27 '25 16:08 Doprez

Thanks !

Eideren avatar Aug 27 '25 16:08 Eideren