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

pool: TaskPool 实现 #50 #57

Merged
merged 17 commits into from
Aug 31, 2022
Merged

Conversation

longyue0521
Copy link
Collaborator

TaskPool管理有限状态机,TaskExecutor负责启动任务,优雅关闭,强制退出等操作

Signed-off-by: longyue0521 longyueli0521@gmail.com

TaskPool管理有限状态机,TaskExecutor负责启动任务,优雅关闭,强制退出等操作

Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Copy link
Contributor

@flycash flycash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我觉得我大概看明白了。我有一些关于使用场景上的疑问:
按照 chanByTask 的逻辑,很显然如果用户得知道自己是不是需要使用 SlowTask。那么问题就在于,如果用户知道哪个是 slow task,它完全可以自己创建一个单独的 pool,然后不是 slow task,他就自己开 goroutine 来解决了。所以这么引入 slow queue 和 fast queue 是为了解决什么问题?

即便真需要解决场景,我觉得这样子更好:

  • 你有一个 pool 的实现,就是来一个我就处理一个,也就是 fast 的,直接开 goroutine
  • 你还有一个 pool 的实现,就是 slow 的,然后按照固定 goroutine 数量来调度
  • 你最终有一个 pool 的实现,就是把两者结合在一起
    我觉得这种设计要比 slow 和 fast 都塞进去一个要好一点。因为我们有非常清晰的三种 pool。

另外关于 panic 的处理,我们可以内部定义个 task 的结构体:

type wrapTask struct {
    t Task
}

func (wrapTask) Run(ctx context.Context) error {
    defer func() {
            // 处理 panic 
    }
    // 后续如果我们暴露了处理 error 的回调,那么可以在这里扩展
}

即 panic 之类的这种问题,我们自己用装饰器来处理掉,那么在 pool 里面就剩下纯粹的调度逻辑了。

.CHANGELOG.md Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
处理Task.Run返回的error,fmt.Println可能导致并发问题

Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Signed-off-by: Longyue Li <longyueli0521@gmail.com>
@codecov
Copy link

codecov bot commented Aug 28, 2022

Codecov Report

Merging #57 (2f91e07) into dev (5b38f6f) will increase coverage by 4.87%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev      #57      +/-   ##
==========================================
+ Coverage   89.29%   94.17%   +4.87%     
==========================================
  Files          12       12              
  Lines         514      669     +155     
==========================================
+ Hits          459      630     +171     
+ Misses         47       31      -16     
  Partials        8        8              
Impacted Files Coverage Δ
pool/task_pool.go 100.00% <100.00%> (+100.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@longyue0521 longyue0521 requested review from flycash and removed request for flycash August 28, 2022 18:22
pool/task_pool.go Show resolved Hide resolved
pool/task_pool_test.go Outdated Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
2.将Task运行时抛出的panic接住并封装为error返回
pool/task_pool.go Outdated Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
pool/task_pool.go Show resolved Hide resolved
pool/task_pool.go Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
pool/task_pool.go Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
2.处理Start/Shutdown/ShutdownNow()重复调用问题
3.Submit方法中只检验task==nil的情况
.CHANGELOG.md Show resolved Hide resolved
2. 重构Shutdown()使用sync.WaitGroup替代用time.Sleep来检测b.num==0
3. 将StartTasks重命名为schedulingTasks
Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Copy link
Contributor

@flycash flycash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

测试比较依赖于 sleep,这个我们后面再来优化它。我稍微有点担心后面在跑 CI 的时候,sleep 一毫秒的情况,可能因为一些乱七八糟的原因,引起了偶发性的失败

.CHANGELOG.md Show resolved Hide resolved
pool/task_pool.go Outdated Show resolved Hide resolved
pool/task_pool_test.go Outdated Show resolved Hide resolved
pool/task_pool_test.go Outdated Show resolved Hide resolved
pool/task_pool_test.go Outdated Show resolved Hide resolved
pool/task_pool_test.go Outdated Show resolved Hide resolved
pool/task_pool_test.go Outdated Show resolved Hide resolved
2.task_pool_test.go 重新组织测试使其更具可读性,清理魔数显示表明其意图等

Signed-off-by: longyue0521 <longyueli0521@gmail.com>
@flycash flycash merged commit 18ed3cf into ecodeclub:dev Aug 31, 2022
@flycash
Copy link
Contributor

flycash commented Aug 31, 2022

我要想想怎么设计一个模糊测试来测试我们这种结构,确保在长期运行的时候不会出现什么乱七八糟的情况

flycash pushed a commit that referenced this pull request Sep 11, 2022
* pool: TaskPool 实现 #50

TaskPool管理有限状态机,TaskExecutor负责启动任务,优雅关闭,强制退出等操作

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 修复ShutdownNow data race问题

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 处理注释等问题

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 省略submitTask接收者名称

* 修改CHANGELOG.md

* 修复因使用go1.19导致的atomic.Int32类型为定义问题
处理Task.Run返回的error,fmt.Println可能导致并发问题

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 重构TaskPool并整理测试

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 修改.CHANGELOG.md

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 1.将time.After替换为time.Sleep
2.将Task运行时抛出的panic接住并封装为error返回

* 1.显示检查queue是否被关闭
2.处理Start/Shutdown/ShutdownNow()重复调用问题
3.Submit方法中只检验task==nil的情况

* 1. 因为调整为重复调用后直接报错,重构ShutdownNow()并去掉submittedTasks和Sync.RWMutex
2. 重构Shutdown()使用sync.WaitGroup替代用time.Sleep来检测b.num==0
3. 将StartTasks重命名为schedulingTasks

* 添加注释修改代码提高可读性

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 1.task_pool.go 修改task的声明方式,显示指定其容量
2.task_pool_test.go 重新组织测试使其更具可读性,清理魔数显示表明其意图等

Signed-off-by: longyue0521 <longyueli0521@gmail.com>

* 改为更宽泛的断言

* 修改测试用例

Signed-off-by: longyue0521 <longyueli0521@gmail.com>
Signed-off-by: Longyue Li <longyueli0521@gmail.com>
@longyue0521 longyue0521 mentioned this pull request Sep 23, 2022
@longyue0521 longyue0521 mentioned this pull request Apr 11, 2023
8 tasks
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.

None yet

3 participants