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

Core: Add mutex to injection resolution #4206

Merged
merged 3 commits into from
Dec 26, 2024
Merged

Core: Add mutex to injection resolution #4206

merged 3 commits into from
Dec 26, 2024

Conversation

yuhan6665
Copy link
Member

  • Turns out we already support async DI resolution regardless of feature ordering
    Previous code contain a race condition causing some resolution is lost
  • Note that the new mutex cover s.pendingResolutions and s.features
    but must not cover callbackResolution() due to deadlock
  • Refactor some method names and simplify code

- Turns out we already support async DI resolution regardless of feature ordering
Previous code contain a race condition causing some resolution is lost
- Note that the new mutex cover s.pendingResolutions and s.features
but must not cover callbackResolution() due to deadlock
- Refactor some method names and simplify code
@yuhan6665 yuhan6665 marked this pull request as draft December 24, 2024 05:08
@RPRX
Copy link
Member

RPRX commented Dec 24, 2024

可以 gkd 赶上这两天再发一版吗

For example OptionalFeatures() is useful for fakedns module
@yuhan6665
Copy link
Member Author

可以 gkd 赶上这两天再发一版吗

应该可以了

@yuhan6665 yuhan6665 marked this pull request as ready for review December 26, 2024 04:44
@RPRX RPRX changed the title Add mutex to injection reolution Core: Add mutex to injection reolution Dec 26, 2024
@RPRX RPRX changed the title Core: Add mutex to injection reolution Core: Add mutex to injection resolution Dec 26, 2024
@RPRX RPRX merged commit 42aea01 into main Dec 26, 2024
36 checks passed
@RPRX
Copy link
Member

RPRX commented Dec 29, 2024

@yuhan6665 群友发现 v24.12.28 有 not all dependencies are resolved 的问题,先 revert 还是?

@RPRX
Copy link
Member

RPRX commented Dec 29, 2024

群友 sakullla 说配置了 balancers 才有问题

@RPRX
Copy link
Member

RPRX commented Dec 29, 2024

我看了下代码,可能是 RoundRobinStrategy 和 RandomStrategy 不应该 require observatory?不知道为啥代码里写的有 fallbackTag 就 require observatory,原来的代码也是,由 #4095 引入,不过是 RequireFeaturesAsync,可能它不会导致报错

总之我认为 RoundRobinStrategy 和 RandomStrategy “有 fallbackTag 就 require observatory”这一逻辑有问题,先删了看看

@RPRX
Copy link
Member

RPRX commented Dec 29, 2024

看了下 RoundRobinStrategy 和 RandomStrategy 的代码是在有 fallbackTag 时用 observatory 来简单判断一下出站是否可用,但这不太符合常理且与这一行为在文档中无记载,找到了 84eeb56 但没找到相关讨论或 PR

如果只是要解决启动报错问题的话可能改成 OptionalFeatures 就行了,但我现在很好奇上面这个 commit 是出于什么原因写的

@RPRX
Copy link
Member

RPRX commented Dec 29, 2024

我想了一下,可能是觉得出站不变的话这个 fallbackTag 无意义,不如赋予它一个意义?@Fangliding 文档需要记载这一行为

如果是这样的话就是预期行为了,还有我看了下这个 fallbackTag 目前似乎是不支持填 balancerTag 的,只能填 outboundTag

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

Successfully merging this pull request may close these issues.

2 participants