GFlisch / Arc4u.Guidance.Doc

Other
5 stars 1 forks source link

Document the unexpected behavior of named services in dependency injection #68

Open vvdb-architecture opened 2 years ago

vvdb-architecture commented 2 years ago

The implementation of named services deviates from expected behavior because they use the default container to register all the implementation types. As a result, differently named services using the same implementation types will refer to the same instance in the scoped and singleton case.

To illustrate why this is bad with the smallest example possible, consider the following interface:

        public interface IRecorder
        {
            void Record(string content);
            public IReadOnlyList<(DateTime Date, string Content)> Playback();
        }

This interface has the following simple implementation:

        public class Recorder: IRecorder
        {
            private readonly List<(DateTime Date, string Content)> _recording = new List<(DateTime Date, string Content)>();

            public void Record(string content) => _recording.Add((DateTime.Now, content));

            public IReadOnlyList<(DateTime Date, string Content)> Playback() => _recording;
        }

Register two services named Task1Recorder and Task2Recorder:

            container.RegisterScoped<IRecorder, Recorder>("Task1Recorder");
            container.RegisterScoped<IRecorder, Recorder>("Task2Recorder");

This is legal. The expected behavior is that we now have two distinct services named Task1Recorder and Task2Recorder with each service their own distinct (scoped) instance.

However, if we now perform one call to Record("...") on each of the instances like this:

            var task1Recorder = container.Resolve<IRecorder>("Task1Recorder");
            var task2Recorder = container.Resolve<IRecorder>("Task2Recorder");

            task1Recorder.Record("This is task 1");
            task2Recorder.Record("This is task 2");

... then the following assertions will fail:

            Assert.Equal(1, task1Recorder.Playback().Count);
            Assert.Equal(1, task2Recorder.Playback().Count);

... because there will be only one instance of IRecorder containing the two Record("...") calls! This is contrary to expected behavior.

In summary, the following unit test fails:

        [Fact]
        public void TestExistingNamedServiceProvider()
        {
            // Arrange
            var container = new ComponentModelContainer();

            container.RegisterScoped<IRecorder, Recorder>("Task1Recorder");
            container.RegisterScoped<IRecorder, Recorder>("Task2Recorder");

            container.CreateContainer();

            // act
            var task1Recorder = container.Resolve<IRecorder>("Task1Recorder");
            var task2Recorder = container.Resolve<IRecorder>("Task2Recorder");

            task1Recorder.Record("This is task 1");
            task2Recorder.Record("This is task 2");

            // assert
            // Both recorders should contain 1 record, but they don't because they refer to the same instance.
            // They will therefore both contain 2 instances and the following asserts will fail
            Assert.Equal(1, task1Recorder.Playback().Count);
            Assert.Equal(1, task2Recorder.Playback().Count);
        }

There is no simple way to solve this, short of a redesign. We might consider adding a test and throw an exception when we see this is happening at registration. This would be better than the silent incorrect behavior we have now.

rdarko commented 1 year ago

could become obsolete in the .net 8 version