search

misskey-dev / misskey

🌎 An interplanetary microblogging platform 🚀
https://misskey-hub.net/
GNU Affero General Public License v3.0
9.84k stars 1.33k forks source link

fix: RateLimiterService #13997

Open anatawa12 opened 3 months ago

anatawa12 commented 3 months ago

What

https://github.com/misskey-dev/misskey/pull/13994#issuecomment-2167357285

とりあえずの対応としてはこれでいいと思ってますが、new Promiseのコールバックが長いことが原因の一端になってると思うので、Limiter.prototype.getをpromise化してそれをasyncで呼ぶ形に近い将来したいと思ったのでちょっと準備してます

を実装しました

Why

What参照

Additional info (optional)

diffとしては大きいですが、小さな作業単位にcommitを分けたので、レビューするときはコミット単位にみてもらうとおそらく見やすいと思います。

13994を先にマージする必要があるため、マージされるまでDraftにしますが Ready for Review です

Checklist

github-actions[bot] commented 3 months ago

このPRによるapi.jsonの差分

差分はこちら ```diff ```

Get diff files from Workflow Page

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 13.33333% with 52 lines in your changes missing coverage. Please review.

Project coverage is 40.07%. Comparing base (8b4933c) to head (bb92129).

Files Patch % Lines
...kages/backend/src/server/api/RateLimiterService.ts 16.32% 41 Missing :warning:
packages/backend/src/server/api/ApiCallService.ts 0.00% 9 Missing :warning:
...ackages/backend/src/server/api/SigninApiService.ts 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #13997 +/- ## =========================================== - Coverage 40.16% 40.07% -0.10% =========================================== Files 1521 1521 Lines 188552 188535 -17 Branches 3509 3512 +3 =========================================== - Hits 75735 75553 -182 - Misses 112246 112439 +193 + Partials 571 543 -28 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

u1-liquid commented 3 months ago

ちょっとoutofscopeかもしれないけどrefactorついでにpolicyでrateLimitFactorを調整すると 回数だけではなく時間も影響されて実際の影響が倍になる問題を直したほうがいいかもしれません (durationとmax両方でfactorをかけ算／割り算するのではなくmaxにだけ割り算した方が良さそう）

KisaragiEffective commented 3 months ago

実際の影響が倍になる

正確には2乗に比例するはずです。

KisaragiEffective commented 3 months ago

mark this pull request as "ready for review": #13994 is merged

anatawa12 commented 3 months ago

ちょっとoutofscopeかもしれないけどrefactorついでにpolicyでrateLimitFactorを調整すると 回数だけではなく時間も影響されて実際の影響が倍になる問題を直したほうがいいかもしれません (durationとmax両方でfactorをかけ算／割り算するのではなくmaxにだけ割り算した方が良さそう）

実装しました

syuilo commented 3 months ago

lint 👀

anatawa12 commented 3 months ago

IDEのeslintがコンフィグのロードに失敗してましたすみません

KisaragiEffective commented 3 months ago

あ、セマンティックコミットじゃない 🙇🏻

syuilo commented 1 month ago

レートリミットのfactorが二回適用されて二乗の効果がある問題を修正

これは意図した挙動だった

anatawa12 commented 1 month ago

これは意図した挙動だった

二乗の効果にした意図は何なのでしょうか

syuilo commented 1 month ago

強さを変更したらdurationも変わる方が自然に思った(別に「factor」は回数のみに影響するとは決まっていない)

anatawa12 commented 1 month ago

強さが線形(2倍にしたら２倍アクセスできる)のほうが私は直感的だと私は思いました。(少なくとも過去の自分は線形の影響度を持つと解釈してました。) 今回の変更では回数を変更しましたが、durtionをfactor倍するのでも私はいいと思っています。

また、両方に影響するべきという話であれば、今の実装以外の線形の方法としてはsqrt(factor)をdurationも回数も影響させるというのが実装可能だと思います。