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

use context with cancel #543

Merged
merged 1 commit into from
Jan 26, 2022

Conversation

DuodenumL
Copy link
Contributor

之前很多地方用的context.TODO() / Background(),在一些注意不到的地方(比如mutex里)可能会产生goroutine阻塞。

主要在rpc的入口处做了处理,保证每个rpc api里用的都是带cancel的context。实际上还有一些地方绕过了rpc而直接调用了cluster / calcium,并且没有使用WithTimeout / WithCancel,修补了一下。

@DuodenumL DuodenumL force-pushed the feature/context-with-cancel branch from 9c583e2 to 38b68cb Compare January 25, 2022 09:38
@DuodenumL DuodenumL force-pushed the feature/context-with-cancel branch from 38b68cb to 5a53b51 Compare January 25, 2022 09:40
@jschwinger233
Copy link
Member

这批改动一个容易犯的错误是对孤儿 goroutine 造成影响.

孤儿 goroutine 对应孤儿进程的的概念, 就是创建者不等 goroutine 运行完自己就先退出了, 像 create 流程最后的 remap 就是孤儿 goroutine.

比如下面这种代码结构, 如果不加分辨地把 context.TODO 改为 CancelContext 并且 defer cancel() 就有问题:

func a() {
    ctx := context.TODO() // 这里改成 ctx, cancel := context.WithCancel(); defer canel() 就有问题
    b(ctx)
}

func b(ctx context.Context) {
    go func() {
        runLongTime(ctx)
    }()
}

@DuodenumL
Copy link
Contributor Author

这批改动一个容易犯的错误是对孤儿 goroutine 造成影响.

孤儿 goroutine 对应孤儿进程的的概念, 就是创建者不等 goroutine 运行完自己就先退出了, 像 create 流程最后的 remap 就是孤儿 goroutine.

比如下面这种代码结构, 如果不加分辨地把 context.TODO 改为 CancelContext 并且 defer cancel() 就有问题:

func a() {
    ctx := context.TODO() // 这里改成 ctx, cancel := context.WithCancel(); defer canel() 就有问题
    b(ctx)
}

func b(ctx context.Context) {
    go func() {
        runLongTime(ctx)
    }()
}

是的,我也相应地检查了这种情况是否存在。对于remap来说,已有的做法是用utils.InheritTracingInfo(ctx, context.TODO())来复制之前的tracing info和peer信息,但是不直接继承自原本的context,也就不会因为ctx cancel导致remap失败。这个InheritTracingInfo在很多地方都有应用,基本上都是出于“复制但不继承”的目的。

@CMGS CMGS merged commit 4f4d5cf into projecteru2:master Jan 26, 2022
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.

3 participants