Faster csv download by shoe116 · Pull Request #44 · BuffettCode/buffett-code-api-client-excel

shoe116 · 2021-11-01T11:04:16Z

to close: #34
非同期処理の制御がうまくいっていなかったために deadlock が起こってしまっていたっぽい

複雑化していた非同期処理を ApiClientCore のみで行うようにする & await の設定をキチンと引き継ぐ
開発者設定から並列実行数の設定を削除

shoe116

@akiomik これで UDF も CSV DLもサクサク動くようになります

shoe116 · 2021-11-01T11:05:46Z

            }
        }
+
+        public void Dispose()


constructor で httpClient を作ってるので、せっかくなので IDisposable にします

shoe116 · 2021-11-01T11:07:04Z

    /// </summary>

-    public class ApiClientCore
+    public class ApiClientCore : IDisposable


こいつだけ非同期で動きます

shoe116 · 2021-11-01T11:08:01Z

-            return response;
+            else
+            {
+                return apiClientCore.Get(request, isConfigureAwait).Result;


この時点で取得を待って動くようにします。当然ながら cache からは常に string が直接返ってくるので、こっちのが全体として違和感がないと思う。

shoe116 · 2021-11-01T11:09:11Z

        }

-        public async Task<JObject> GetQuarter(string ticker, FiscalQuarterPeriod period, bool useOndemand, bool isConfigureAwait = true, bool useCache = true)
+        public JObject GetQuarter(string ticker, FiscalQuarterPeriod period, bool useOndemand, bool isConfigureAwait = true, bool useCache = true)


ApiClientCoreWithCache が同期的に動くようになったので、ここも Json をそのまま返します

shoe116 · 2021-11-01T11:10:08Z

        }

-        private void textAPIKey_TextChanged(object sender, EventArgs e)
+        private void TextAPIKey_TextChanged(object sender, EventArgs e)


命名規則で怒られてたので、大文字始まりにしておきました

shoe116 · 2021-11-01T11:10:34Z

            textAPIKey.Text = setting.ApiKey;
            checkDebugMode.Checked = setting.DebugMode;
            checkIsOndemandEndpointEnabled.Checked = setting.IsOndemandEndpointEnabled;
-            var maxDegreeOfParallelism = setting.MaxDegreeOfParallelism;


並行実行を渡す開発者オプションを削除します

shoe116 · 2021-11-01T11:11:30Z

        public static PeriodSupportedTierResolver Create(IBuffettCodeApiClient apiClient, IApiResponseParser parser) => new PeriodSupportedTierResolver(apiClient, parser, new SupportedTierDictionary());

-        public SupportedTier Resolve(DataTypeConfig dataType, string ticker, IPeriod period)
+        public SupportedTier Resolve(DataTypeConfig dataType, string ticker, IPeriod period, bool isConfigureAwait, bool useCache)


isConfigureAwait と useCache を外から渡せるように変更しています

shoe116 · 2021-11-01T11:12:10Z

-            Assert.AreEqual(SupportedTier.OndemandTier, helper.FindAvailableTier(DataTypeConfig.Quarter, ticker, ondemandOldest.Next() as FiscalQuarterPeriod, true));
-            Assert.AreEqual(SupportedTier.OndemandTier, helper.FindAvailableTier(DataTypeConfig.Quarter, ticker, ondemandLatest, true));
-            Assert.ThrowsException<NotSupportedTierException>(() => helper.FindAvailableTier(DataTypeConfig.Quarter, ticker, ondemandLatest.Next() as FiscalQuarterPeriod, true));
+            Assert.AreEqual(SupportedTier.FixedTier, helper.FindAvailableTier(DataTypeConfig.Quarter, ticker, fixedOldest, true, true, true));


test は両方とも true の、 UDF と同じ仕様で動かします

CSVダウンロードように、 ConfigureAwait(false) のケースもテストしたほうがいいと思う

shoe116 · 2021-11-01T11:15:34Z

            var quarters = PeriodRange<FiscalQuarterPeriod>.Slice(parameters.Range, 12)
                            .SelectMany
-                            (r => processor.GetApiResources(DataTypeConfig.Quarter, parameters.Ticker, r.From, r.To, true, true)
+                            (r => processor.GetApiResources(DataTypeConfig.Quarter, parameters.Ticker, r.From, r.To, false, true)


ここを true で渡すと、リボンから呼ぶとすごく遅くなるので false にします
https://devblogs.microsoft.com/dotnet/configureawait-faq/#why-would-i-want-to-use-configureawaitfalse

ConfigureAwait(true) にしたいケースがほとんど存在しないし (テストが書きやすいくらい？)、基本的に ConfigureAwait(false) でハードコードしたほうが安全な気がしてる

UDFのほうは考慮する必要があるか

shoe116 · 2021-11-01T11:39:02Z

-        /// <summary>
-        /// 最大同時実行数のデフォルト値
-        /// </summary>
-        private static readonly uint MaxDegreeOfParallelismDefault = 8;


最大同時実行数を設定から消します

akiomik

単体テストで実APIを呼び出すのは単体テストの責務から外れてるように感じるので、モックを使うように書き直して欲しいです 🙏

shoe116 · 2021-11-03T06:46:13Z

@akiomik 異常系については mock にできました。 b1717a3
正常系はだいぶ書き直しが必要なので、このPRを merge、リリース後下記で取り組みます
#45

shoe116

@akiomik ApiClientCore を mock できるようにしました

shoe116 · 2021-11-03T06:47:04Z

    /// </summary>

-    public class ApiClientCore : IDisposable
+    public class ApiClientCore : IDisposable, IApiClientCore


実際にhttp通信を行うこいつを差し替えられるようにします

shoe116 · 2021-11-03T06:47:54Z

+            return new ApiClientCoreWithCache(apiClientCore, cacheHelper);
        }

-        public static ApiClientCoreWithCache Create(string apiKey, Uri baseUrl, MemoryCache cache)


この Create は誰も使ってなかったので削除

shoe116 · 2021-11-03T06:48:02Z

        }

-        public static ApiClientCoreWithCache Create(string apiKey, Uri baseUrl, MemoryCache cache)
+        public static ApiClientCoreWithCache Create(IApiClientCore apiClientCore, ApiRequestCacheHelper cacheHelper)


代わりにこっちを作りました

shoe116 · 2021-11-03T06:49:01Z

+            var mockApiClientCore = new ErrorMockApiClientCore("dummy");
+            var client = ApiClientCoreWithCache.Create(mockApiClientCore, CreateCheHelperForTest());
+            ApiGetRequest request = new ApiGetRequest("dummy endpoint", new Dictionary<string, string>());
+            Assert.ThrowsException<BuffettCodeApiClientException>(() => client.Get(request, false, false));


clientCore が例外を投げた場合はそのまま投げることを確認しています

shoe116 · 2021-11-03T06:49:19Z

+            Assert.AreEqual(request.ToString(), client.Get(request, false, true));
+
+            // don't use cache
+            Assert.AreEqual(request.ToString(), client.Get(request, false, false));


cahceを使っても使わなくても、同じ結果が返ることを確認しています

shoe116 · 2021-11-03T06:49:31Z

+            var mockApiClientCore = new MockApiclientCore("dummy");
+            var client = ApiClientCoreWithCache.Create(mockApiClientCore, CreateCheHelperForTest());
+            ApiGetRequest request = new ApiGetRequest("dummy endpoint", new Dictionary<string, string>());
+            Assert.AreEqual(request.ToString(), client.Get(request, false, true));


1回目のリクエスト

shoe116 · 2021-11-03T06:50:04Z

    [TestClass()]
    public class ApiClientCoreWithCacheTests
    {
+        private static ApiRequestCacheHelper CreateCheHelperForTest()


test ごとにcache helper を作ります。

akiomik · 2021-11-03T06:54:20Z

-                return await response.Content.ReadAsStringAsync().ConfigureAwait(isConfigureAwait);
+                var content = response.Content.ReadAsStringAsync().Result;


ReadAsStringAsync().Result って危険な呼び出しにみえるけど、非同期処理が未完了だったらブロックしてくれると。

https://docs.microsoft.com/ja-jp/dotnet/api/system.threading.tasks.task-1.result?view=net-5.0#--

#44 (comment)

akiomik · 2021-11-03T07:07:20Z

+{
+    public interface IApiClientCore
+    {
+        Task<string> Get(ApiGetRequest request, bool isConfigureAwait);


using (var response = await httpClient.GetAsync(path).ConfigureAwait(isConfigureAwait)) しているのでシグニチャは Task<string> のまま、と

これどうするかすごい迷ったけど、この ApiClientCore の通信部分は非同期に呼び出す前提にしておいたほうがいいと思う。

string 直接返す client が Task<string> に変換して返すのは全然難しくないし

将来的には非同期クライアントになってるほうがいいと思うので、内部的には半同期みたいな感じでもシグニチャはこのままでいいと思う

akiomik · 2021-11-03T07:36:25Z

-            Assert.ThrowsExceptionAsync<InvalidAPIKeyException>
-                (() => client.GetDaily("6502", day, true, true, false));


なるほど、こういうのがそのまま投げてるテストケースか

そそ、 http 通信のlogic が処理の真ん中に入っちゃってるんだよね。そとから clientCore を渡す形の VxApiClientHelper みたいなやつを作って、各 client はその static な関数を呼ぶだけにしようと思う

akiomik · 2021-11-03T07:44:04Z

+        private void CheckDebugMode_CheckedChanged(object sender, EventArgs e)
+        {
+
+        }


これは勝手に定義されてるやつ？

そう、formのUIをいじると勝手に作られる event handler 用の関数

shoe116 added 6 commits November 1, 2021 19:02

wip: firster csv

dbf2ece

remove useless async

36e3693

remove parallelism

d76f549

reformat

42e15e7

remove useless settings form ribbon form design

90c39ac

Merge remote-tracking branch 'origin/master' into faster_csv_download

c0f272a

shoe116 commented Nov 1, 2021

View reviewed changes

shoe116 requested a review from akiomik November 1, 2021 11:31

shoe116 commented Nov 1, 2021

View reviewed changes

akiomik suggested changes Nov 3, 2021

View reviewed changes

make IApiClientCore and using mock for ApiClientCoreWithCache

b1717a3

shoe116 mentioned this pull request Nov 3, 2021

各種 API Client の test を mock化する #45

Closed

shoe116 commented Nov 3, 2021

View reviewed changes

akiomik reviewed Nov 3, 2021

View reviewed changes

Comment thread BuffettCodeAPIClient/ApiClientCore.cs Outdated

fix for review:

1f6ce5a

#44 (comment)

akiomik reviewed Nov 3, 2021

View reviewed changes

akiomik approved these changes Nov 3, 2021

View reviewed changes

shoe116 merged commit 56504a4 into master Nov 3, 2021

shoe116 deleted the faster_csv_download branch November 3, 2021 07:59

shoe116 mentioned this pull request Nov 3, 2021

release 1.0.1.4 #46

Merged

1 task

		return await response.Content.ReadAsStringAsync().ConfigureAwait(isConfigureAwait);
		var content = response.Content.ReadAsStringAsync().Result;

		Assert.ThrowsExceptionAsync<InvalidAPIKeyException>
		(() => client.GetDaily("6502", day, true, true, false));

Conversation

shoe116 commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shoe116 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoe116 Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoe116 Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akiomik left a comment

Choose a reason for hiding this comment

Uh oh!

shoe116 commented Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shoe116 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shoe116 Nov 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

shoe116 commented Nov 1, 2021 •

edited

Loading

shoe116 Nov 1, 2021 •

edited

Loading

shoe116 Nov 1, 2021 •

edited

Loading

shoe116 commented Nov 3, 2021 •

edited

Loading

shoe116 Nov 3, 2021 •

edited

Loading