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: 重构去除 OnDemandBlockTaskPool 中的 context.Context 字段 #65

Closed
Tracked by #175
flycash opened this issue Sep 1, 2022 · 5 comments
Closed
Tracked by #175

Comments

@flycash
Copy link
Contributor

flycash commented Sep 1, 2022

仅限中文

当前实现缺陷

目前的 OnDemandBlockTaskPool 的定义里面使用了 context.Context 来作为字段类型,核心是用它来作为关闭的通知:

type OnDemandBlockTaskPool struct {
	// TaskPool内部状态
	state int32

	queue chan Task
	token chan struct{}
	num   int32
	wg    sync.WaitGroup

	// 外部信号
	done chan struct{}
	// 内部中断信号
	ctx        context.Context
	cancelFunc context.CancelFunc
}

但是从语义上来说,context.Context 所表达的是一个上下文概念,一般用作方法参数。即便不是用作方法参数,也是用在 http.Request 这种,和请求生命周期保持一致的场景中。

TaskPool 不同于此。TaskPool 的生命周期可以认为是极其漫长的,大多数的使用场景都是在应用或者服务启动的时候创建一个,而后在服务或者应用关闭的时候销毁。也就是说,它会跨越多个请求。

在这种场景之下,使用 context.Context 作为参数则不太合适了。

重构方案

描述可以如何重构,以及重构之后带来的效果,如可读性、性能等方面的提升

我们可以考虑使用简单的 channel 来发送关闭信号:

type OnDemandBlockTaskPool struct {
	// TaskPool内部状态
	state int32

	queue chan Task
	token chan struct{}
	num   int32
	wg    sync.WaitGroup

	// 外部信号
	done chan struct{}
	// 内部中断信号
	closing chan struct{}
}

其它

任何你觉得有利于解决问题的补充说明

要注意,最好将容量设置为1,并且在发送了关闭信号之后,要把 channel 给关闭掉

你使用的是 ekit 哪个版本?

你设置的的 Go 环境?

上传 go env 的结果

@longyue0521
Copy link
Collaborator

longyue0521 commented Sep 1, 2022

之所以用ctx是想表达在成功调用ShutdownNow那一刻,所有处于“可中断上下文”中的go协程均能收到中断信号.这其中不仅包括了schedulingTasks任务调度循环,还包括那些运行中且内部监听了ctx信号的Task

func (b *OnDemandBlockTaskPool) schedulingTasks() {
                                  // .....

				// 这里向task传递的正是那个ctx,对于那些在内部监听了ctx.Done()的Task
                                  // 使其以自定义方式更快速地中断运行
				err := task.Run(b.ctx)
				if err != nil {
					return
				}
			}()
		}
	}
}

https://github.com/gotomicro/ekit/blob/dev/pool/task_pool.go#L253

@flycash
Copy link
Contributor Author

flycash commented Sep 1, 2022

👍
我试了一下别的方式,比如说使用 channel,确实不如这种方法好。

@kangdan6
Copy link
Contributor

@longyue0521 @flycash 有几点疑问,请大佬指点:
1、NewOnDemandBlockTaskPool时用户必须使用WithQueueBacklogRate这个options来设置queueBacklogRate吗?否则if判断queueBacklogRate默认值为0,会创建失败吧?

2、372行注释不对吧(刚启动的协程除非恰巧赶上Shutdown/ShutdownNow被调用,否则应该至少执行一个task),如果initGo是10,用户只提交了2个任务,那剩余8个goroutine不会执行任务,所以不会至少执行一个task吧

@longyue0521
Copy link
Collaborator

@kangdan6

  1. 既然设置为options,必然是可选的.即用户不通过WithXXXOption指定,在New的时候会提供默认值. 你可以看一下对应的测试代码,也可以在本地构造一个不设置queueBacklogRate而使NewOnDemandBlockTaskPool失败的测试用例
  2. 这个注释是用来说明,为什么刚刚创建idleTimer就要将其关闭,因为刚创建出来的协程很可能直接走for循环中的case <-idleTimer.C分支而退出. 那么这样耗时、耗资源创建出的协程却什么都没干是没有意义的.你举的用例是因为提供足够的任务,但也确实反映出该注视确实会引起歧义.你可以提交一个issue修改/完善该注释.

@kangdan6
Copy link
Contributor

kangdan6 commented Jan 16, 2023 via email

@longyue0521 longyue0521 mentioned this issue 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

No branches or pull requests

3 participants