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

ConfigLoader - add additional params #13

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 25 additions & 9 deletions Runtime/Snipe.Unity/Config/SnipeConfigLoader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,9 @@ public SnipeConfigLoader(string projectID, IApplicationInfo appInfo)
_url = "https://config.snipe.dev/api/v1/configStrings";
}

public async UniTask<Dictionary<string, object>> Load()
public async UniTask<Dictionary<string, object>> Load(Dictionary<string, object> additionalParams = null)
{
string requestParamsJson = "{" +
$"\"project\":\"{_projectID}\"," +
$"\"deviceID\":\"{_appInfo.DeviceIdentifier}\"," +
$"\"identifier\":\"{_appInfo.ApplicationIdentifier}\"," +
$"\"version\":\"{_appInfo.ApplicationVersion}\"," +
$"\"platform\":\"{_appInfo.ApplicationPlatform}\"," +
$"\"packageVersion\":\"{PackageInfo.VERSION_CODE}\"" +
"}";
string requestParamsJson = BuildRequestParamsJson(additionalParams);

Dictionary<string, object> config = null;

Expand Down Expand Up @@ -81,5 +74,28 @@ public async UniTask<Dictionary<string, object>> Load()

return config;
}

private string BuildRequestParamsJson(Dictionary<string, object> additionalParams)
{
var requestParams = new Dictionary<string, object>
{
{ "project", _projectID },
{ "deviceID", _appInfo.DeviceIdentifier },
{ "identifier", _appInfo.ApplicationIdentifier },
{ "version", _appInfo.ApplicationVersion },
{ "platform", _appInfo.ApplicationPlatform },
{ "packageVersion", PackageInfo.VERSION_CODE }
};

if (additionalParams != null)
{
foreach (var param in additionalParams)
{
requestParams[param.Key] = param.Value;
}
}

return JSON.ToJSON(requestParams);
}
}
}
43 changes: 37 additions & 6 deletions Runtime/Snipe.Unity/Config/SnipeConfigLoadingService.cs
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
using System.Collections.Generic;
using System.IO;
using System.Threading;
using Cysharp.Threading.Tasks;
using fastJSON;
using UnityEngine;

namespace MiniIT.Snipe
{
public interface ISnipeConfigLoadingService
{
Dictionary<string, object> Config { get; }
UniTask<Dictionary<string, object>> Load(CancellationToken cancellationToken = default);
UniTask<Dictionary<string, object>> Load();
void Reset();
}

public class SnipeConfigLoadingService : ISnipeConfigLoadingService
Expand All @@ -20,12 +24,14 @@ public class SnipeConfigLoadingService : ISnipeConfigLoadingService
private SnipeConfigLoader _loader;
private readonly string _projectID;

private CancellationTokenSource _loadingCancellation;
Copy link
Contributor

Choose a reason for hiding this comment

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

не всё так просто...
если класс содерит disposable поля, то и сам класс должен быть IDisposable со всеми вытекающими


public SnipeConfigLoadingService(string projectID)
{
_projectID = projectID;
}

public async UniTask<Dictionary<string, object>> Load(CancellationToken cancellationToken = default)
public async UniTask<Dictionary<string, object>> Load()
{
if (_config != null)
{
Expand All @@ -34,29 +40,54 @@ public async UniTask<Dictionary<string, object>> Load(CancellationToken cancella

if (_loading)
{
await UniTask.WaitWhile(() => _config == null, PlayerLoopTiming.Update, cancellationToken);
await UniTask.WaitWhile(() => _config == null, PlayerLoopTiming.Update, _loadingCancellation.Token);
return _config;
}

_loadingCancellation ??= new CancellationTokenSource();
Copy link
Contributor

Choose a reason for hiding this comment

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

во-первых, нельзя игнорировать уже имеющийся токен

_loadingCancellation = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);

во-вторых, у тебя тут есть проверка на случай, если _loadingCancellation != null это хорошо, но нет никакого действия. Если у нас уже есть какой-то ранее созданный CTS, значит мы уже запустили загрузку и она сейчас уже в процессе - требуется принять решение, как с ней поступить: либо отменить старую, либо отменить новую и ждать окончания старой

_loading = true;

if (_loader == null)
{
await UniTask.WaitUntil(() => SnipeServices.IsInitialized, PlayerLoopTiming.Update, cancellationToken)
await UniTask.WaitUntil(() => SnipeServices.IsInitialized, PlayerLoopTiming.Update, _loadingCancellation.Token)
Copy link
Contributor

Choose a reason for hiding this comment

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

так можно, но только если используется LinkedTokenSource (см. выше), иначе мы игнорируем внешний cancellationToken, а он по логике главней нашего внутреннего

.SuppressCancellationThrow();

if (cancellationToken.IsCancellationRequested)
if (_loadingCancellation.Token.IsCancellationRequested)
Copy link
Contributor

Choose a reason for hiding this comment

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

сохрани токен в локальную переменную вместо того, чтобы каждый раз вызывать свойство _loadingCancellation.Token
обращения к свойствам - это фактически вызов метода, а это значит, что под капотом выполняется целая куча промежуточных действий с сохранением стека и сменой контекста - можно оптимизировать одной строкой

{
return _config;
}

_loader ??= new SnipeConfigLoader(_projectID, SnipeServices.ApplicationInfo);
}

_config = await _loader.Load();
var loadedAdditionalParams = await LoadAdditionalParamsAsync();

_config = await _loader.Load(loadedAdditionalParams);
_loading = false;

return _config;
}

public void Reset()
{
_loadingCancellation.Cancel();
_loadingCancellation.Dispose();
_loadingCancellation = null;
_config = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

сначала нужно остановить загрузку (которая теоретически может выполняться в данный момент) через CancellationToken. иначе она завершится в параллельном потоке и присвоит своё значение конфигу вне зависимости от того, что тут ему присваивается null

_loading = false;
}

private async UniTask<Dictionary<string, object>> LoadAdditionalParamsAsync()
{
string filePath = Path.Combine(Application.persistentDataPath, string.Format("additionalParams{0}.json", Application.version));

if (!File.Exists(filePath))
{
return new Dictionary<string, object>();
}

string json = await File.ReadAllTextAsync(filePath);
return (Dictionary<string, object>)JSON.Parse(json);
}
}
}