Skip to content
This repository has been archived by the owner on Apr 12, 2023. It is now read-only.

HttpClientの取り回しを再検討 #713

Open
keiji opened this issue Jan 14, 2022 · 10 comments · May be fixed by #951
Open

HttpClientの取り回しを再検討 #713

keiji opened this issue Jan 14, 2022 · 10 comments · May be fixed by #951
Assignees

Comments

@keiji
Copy link
Collaborator

keiji commented Jan 14, 2022

#684 の対応で、都度HttpClientのインスタンスを作成、検討する方式にしたものの、再利用が原則であるとの指摘あり(同Issueコメント)。

https://docs.microsoft.com/ja-jp/dotnet/api/system.net.http.httpclient?view=net-5.0

HttpClient は、1回インスタンス化し、アプリケーションの有効期間全体に再利用することを目的としています。
すべての要求に対して HttpClient クラスをインスタンス化すると、大量の読み込みで使用可能なソケットの数が枯渇します。

実際にSocketが枯渇するかどうかは分かりませんが、HttpClient を都度作り直すのは推奨されないように思います。

Originally posted by @b-wind in #684 (comment)


Timeout再設定時のInvalidOperationExceptionは必ず起きるので対応しなければならないものの、ソケット枯渇のような潜在バグについても潰しておく必要がある。

  • HttpClientはなるべく共有した方が良い
  • (一度接続したら)Timeoutのような一部の設定は変更できない(コネクションプールを保持しているから?)

最良の方式はIHttpClientServiceに目的別のHttpClient(ダウンロード用、APIアクセス用など)のインスタンスを保持して各サービスに割り振ること(#707 で提案)

けどこれは変更が大きくなるので、今回は単純に各サービスが1つのHttpClientを保持する。Timeout設定が一回で済むようにインスタンス化(コンストラクタ?)で実行。としたい。

HttpClientをusingで囲わないでください
https://qiita.com/superriver/items/91781bca04a76aec7dc0

これも対応する。

@keiji keiji self-assigned this Jan 14, 2022
@keiji keiji added the COCOA2 label Jan 14, 2022
@b-wind
Copy link

b-wind commented Jan 14, 2022

ドキュメントには、
https://docs.microsoft.com/ja-jp/dotnet/api/system.net.http.httpclient.timeout?view=net-5.0

また、タスクで を使用して、個々の要求に対して異なるタイムアウト CancellationTokenSource を設定できます。

(日本語がおかしいのはご愛敬)

とあります。

また、サンプルコードとして以下の物があります。
https://docs.microsoft.com/ja-jp/dotnet/csharp/programming-guide/concepts/async/cancel-async-tasks-after-a-period-of-time#complete-example

サンプルコード中では、ループ内で CancellationTokenSource を使い回していますが CancellationTokenSource 自体はリクエスト毎に都度使い捨てで問題ないかと思います。

C# 的にこれがベストな方法かは判断が付きかねますが、実装の一案ではありそうです。

@b-wind
Copy link

b-wind commented Jan 14, 2022

(一度接続したら)Timeoutのような一部の設定は変更できない(コネクションプールを保持しているから?)

こちらについては以下の様な記述があります。
現実的には別スレッドで動作中だったり、 Keepalive 等でコネクション自体が繋がったまま変更されるのを防ぎたいなどの理由を推測します。

未処理の HttpClient 要求がある間は、のプロパティを変更しないでください。これはスレッドセーフではないためです。

@b-wind
Copy link

b-wind commented Jan 14, 2022

個人的には RequestHeader や Timeout の値が異なる少数の HttpClient を保持する分には許容されるのでは無いかと考えますが、こちらは識者の意見が欲しい所です。

@keiji
Copy link
Collaborator Author

keiji commented Jan 14, 2022

これについては段階的に直していきます。
まずはusingを止めて、各ServiceでHttpClientを1つだけ持つようにします( #714 )。

まずは現時点で分かっていることとして、現在の実装では using で毎回廃棄しているので、都度disposeされる。disposeされるとコネクションプールのソケットは閉じられるという認識で、今回のアプリを仮に配信したとして、すぐにソケットを枯渇させると言うことはなさそうです。

一方、コネクションプールの廃棄はとてもコストの高い処理になりますので、現在のdevelopの実装がイケてない、非効率的なものという点で修正の優先順位は高いです(この実装でリリースはしない)。

根本的にはHttpClientServiceから各Serviceに必要な設定のHttpClientを割り振る予定です。
もしHttpClientのインスタンスが1つにできるならそれが理想で、HTTP Headerや、タイムアウトの時間を設定する関係でHttpClientインスタンスそのものを分ける必要がある。分けた方が合理的となればそうしようと考えています。

このIssueはOpenのままにしておくので、なにか知見があればお寄せいただければ。

@b-wind
Copy link

b-wind commented Jan 14, 2022

これについては段階的に直していきます。

御意。
HttpClient を多用するアプリではないので、ただちに問題になることは無いだろうと言う認識です。
とは言え何かの切っ掛けで問題のトリガーになりかねないので早めに改善しておきたいですね。

@b-wind
Copy link

b-wind commented Feb 7, 2022

現在の実装では using で毎回廃棄しているので、都度disposeされる。disposeされるとコネクションプールのソケットは閉じられるという認識

情報共有の為に記述しておきます。

HttpClient の dispose で TCP/IP のソケットは解放されることが期待されます。
ただし、実際にどのタイミングで解放されるかはOSのTCP/IPスタックの実装依存で、 Windows 等だと数分間 TIME_WAIT と言う状態で残っているのはよく見かけます。( .NET に限らない )
Android / iOS で具体的にどのように扱われるかは分からないのですが。

またクライアントリソースの問題だけで無く、ファイヤーウォールやNAPTのセッション上限に達する可能性もありますので、問題が発生すると原因にたどり着くのが困難になることも想定されます。

杞憂であるとは考えていますが、そう言うこともあり得ると言う事は覚えておいて戴きたいかなと思いました。

@keiji
Copy link
Collaborator Author

keiji commented Feb 8, 2022

やっぱり、Singletonなサービスにタイプ別のHttpClientを3つくらい保持して、それぞれのサービスから使う。ってのが一番良さそうですね。

@b-wind
Copy link

b-wind commented Feb 8, 2022

Singletonなサービスにタイプ別のHttpClientを3つくらい保持して、それぞれのサービスから使う。

はい。現状では無難な解決法だと思います。

@b-wind
Copy link

b-wind commented Feb 9, 2022

補足として、MS的には IHttpClientFactory を利用する事を推奨しているようです。
https://docs.microsoft.com/ja-jp/dotnet/architecture/microservices/implement-resilient-applications/use-httpclientfactory-to-implement-resilient-http-requests

実際試している人の記事を見るとパフォーマンスも含めて良好な様子。
https://www.kinakomotitti.net/entry/2021/04/03/164718

ただ、これはこれできちんと調べないと罠がありそうなので、今後の課題かなと言う印象です。

@keiji
Copy link
Collaborator Author

keiji commented Mar 31, 2022

#938 にも関わるので、これやります。

@keiji keiji linked a pull request Mar 31, 2022 that will close this issue
1 task
@keiji keiji removed the COCOA2 label Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants