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

Async Binding Support #155

Merged
merged 19 commits into from
Feb 24, 2021

Conversation

altunsercan
Copy link

@altunsercan altunsercan commented Jul 5, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • No compiler errors or warnings

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Issue Number

Issue Number: #153

Create or search an issue here: Extenject/Issues

What is the current behavior?

  • Currently Extenject does not support async bindings. By design, dependency injection injects only once to a target object. So there is no reinjection after an async resource is ready.
  • However there is LazyInject<T>. An intermediary object is injected instead of type T, so in implementation binding can be resolved later when the value is actually needed.

What is the new behavior?

  • Adds a AsyncInject<T> similar to LazyInject<T>. However unlike LazyInject<T> this is not automatic. The dependency needs to be bound explicitly through a specific binder.
  • Added Container.BindAsync<T>().FromMethod( async () => {...}) binding format for binding async dependencies
  • Everything is put into optional extras so feature can be excluded.
  • Added AddressableInject as subclass of AsyncInject. Provides access to handle which is used in Addressable system to release a loaded resource from memory after use.

Does this introduce a breaking change?

  • Yes
  • No

Other information

  • At first I wanted to use Container.Bind<T>().FromAsyncMethod( async ()=> {...}). While it is technically possible to do it this way, I felt that it is better to be explicit. Also since feature is not supporting factories or memory pools yet, it may have cause confusion or bugs. So I kept it separate.

On which Unity version has this been tested?

  • 2020.4 LTS
  • 2020.3
  • 2020.2
  • 2020.1
  • 2019.4 LTS
  • 2019.3
  • 2019.2
  • 2019.1
  • 2018.4 LTS

Scripting backend:

  • Mono
  • IL2CPP

Note: Every pull request is tested on the Continuous Integration (CI) system to confirm that it works in Unity.

Ideally, the pull request will pass ("be green"). This means that all tests pass and there are no errors. However, it is not uncommon for the CI infrastructure itself to fail on specific platforms or for so-called "flaky" tests to fail ("be red"). Each CI failure must be manually inspected to determine the cause.

CI starts automatically when you open a pull request, but only Releasers/Collaborators can restart a CI run. If you believe CI is giving a false negative, ask a Releaser to restart the tests.

@altunsercan
Copy link
Author

An extra information for Addressables. I have tested addressable loading in an additional test. This is included as a zip folder inside Async tests. I didn't distribute it outside zip folder because that would require projects to import addresables package. It contains following test file that shows how to load addressables with this feature.

using System;
using Zenject;
using System.Collections;
using NUnit.Framework;
using UnityEngine;
using UnityEngine.AddressableAssets;
using UnityEngine.ResourceManagement.AsyncOperations;
using UnityEngine.ResourceManagement.ResourceLocations;
using UnityEngine.TestTools;

public class TestAddressable : ZenjectIntegrationTestFixture
{
    
    [UnityTest]
    public IEnumerator TestAddressableAsyncLoad()
    {
        PreInstall();
        AsyncOperationHandle<GameObject> handle = default;
        Container.BindAsync<GameObject>().FromMethod(async () =>
        {
            try
            {
                var locationsHandle = Addressables.LoadResourceLocationsAsync("TestAddressablePrefab");
                await locationsHandle.Task;
                Assert.Greater(locationsHandle.Result.Count, 0, "Key required for test is not configured. Check Readme.txt in addressable test folder");

                IResourceLocation location = locationsHandle.Result[0];
                handle = Addressables.LoadAsset<GameObject>(location);
                await handle.Task;
                return handle.Result;
            }
            catch (InvalidKeyException)
            {
                
            }
            return null;
        }).AsCached();
        PostInstall();

        yield return null;
            
        AsyncInject<GameObject> asycFoo = Container.Resolve<AsyncInject<GameObject>>();

        int frameCounter = 0;
        while (!asycFoo.HasResult && !asycFoo.IsFaulted)
        {
            frameCounter++;
            if (frameCounter > 10000)
            {
                Addressables.Release(handle);
                Assert.Fail();
            }
            yield return null;    
        }
            
        Addressables.Release(handle);
        Assert.Pass();
    }   
}

@Mathijs-Bakker Mathijs-Bakker self-requested a review July 6, 2020 20:06
@Mathijs-Bakker Mathijs-Bakker added the enhancement New feature or request label Jul 6, 2020
@altunsercan
Copy link
Author

Added awaiter support as suggested in gitter group and test related to it. And simple documentation page. However did not update main RaidMe page.

@altunsercan
Copy link
Author

@Mathijs-Bakker Added typeless base AsyncInject so that injections can be grouped in list bindings List<AsyncInject>. Added a sample test case that uses DecoratableKernel to delay initialization until all AsyncInjects are completed.

CI tests failed, but I am suspecting it is unrelated.

My intention is to make final addition with Addressable flavored FromAddressable() providers. Then I will ask for the review.

Thanks.

@altunsercan altunsercan changed the title Work In Progress: Async Binding Support Async Binding Support Jul 20, 2020
@altunsercan
Copy link
Author

@Mathijs-Bakker Feature is ready for review. Final Addressable addition is not added to the documentation. I can finalize that after the review.

@altunsercan
Copy link
Author

Based on comment in gitter, I updated addressable method to manually throw error when addressable fails to load.

By default addressable return handle.Status == AsyncOperationStatus.Failed instead of throwing exception. AsyncInject does not consider null values as failure state, since you may have valid async operations that return null. Instead added try catch to the AsyncInject and set IsFaulted when any async operation throws an exception.

Copy link
Collaborator

@RichardWepner RichardWepner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked at the code and mentioned everything I noticed. This isn't a lot, since I'm currently not very deep into the internal implementation of Extenject.

Besides that, I made some suggestions for the documentation in order to improve the grammar a bit. Also, I'd use the async keyword as a keyword, and thus with the Markdown formatting for inline code (example: async).

All of the suggestions are up for debate.

Documentation/Async.md Outdated Show resolved Hide resolved
Documentation/Async.md Outdated Show resolved Hide resolved

In dependency injection, the injector resolves dependencies of the target class only once, often after class is first created. In other words, injection is a one time process that does not track the injected dependencies to update them later on. If a dependency is not ready at the moment of injection; either binding would not resolve in case of optional bindings or fail completely throwing an error.

This is creates a dilemma when implementing dependencies that are resolved asyncroniously. You can design around the DI limitations by carefully architecting your code so that the injection happens after async process is completed. This requires careful planning, increased complexity in setup. It is also prone to errors.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This is creates a dilemma when implementing dependencies that are resolved asyncroniously. You can design around the DI limitations by carefully architecting your code so that the injection happens after async process is completed. This requires careful planning, increased complexity in setup. It is also prone to errors.
This creates a dilemma while implementing dependencies that are resolved asynchronous. You can design around the DI limitations by carefully designing your code so that the injection happens after the `async` process is completed. This requires careful planning, which leads to an increased complexity in the setup, and is also prone to errors.

Documentation/Async.md Outdated Show resolved Hide resolved
Documentation/Async.md Outdated Show resolved Hide resolved
Documentation/Async.md Outdated Show resolved Hide resolved
Documentation/Async.md Outdated Show resolved Hide resolved
Documentation/Async.md Outdated Show resolved Hide resolved
Documentation/Async.md Outdated Show resolved Hide resolved
@altunsercan
Copy link
Author

@RichardWepner thanks for the review. I applied all suggestions. Did not apply documentation changes directly here to have single commit. So I did those locally too.

@Mathijs-Bakker Mathijs-Bakker merged commit 5d899ec into modesttree:master Feb 24, 2021
@Mathijs-Bakker
Copy link
Collaborator

🙇

This was linked to issues Feb 24, 2021
@CharlieHess
Copy link

Did this ever make it into the Unity asset store package? I'm on the latest version (9.2.0) and there's no OptionalExtras/Async folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Async Support for bindings Unity Addressable Assets
4 participants