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

syncx: sync.Cond的超时等待版,Cond.WaitWithContext(ctx) #192

Merged
merged 14 commits into from
Aug 10, 2023

Conversation

fifth-month
Copy link
Contributor

@fifth-month fifth-month commented Jun 12, 2023

整体思路是每一个等待的对象都创建一个chan,然后监听chan和ctx的信号;被通知时,如果先进入了超时分支,但同时被signal通知时,由于select会随机选择一个,但是被signal通知也就意味着成功了,只是同时超时了而已,所以超时后需要先加锁,然后再判断是否被通知过,如果被通知过,就直接返回,否则进入超时的真正流程,移除等待的对象。

@longyue0521
Copy link
Collaborator

@fifth-month 根据 https://ekit.gocn.vip/contribution/ 修一下下面的错误

@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #192 (64da401) into dev (b656686) will increase coverage by 0.11%.
The diff coverage is 95.65%.

@@            Coverage Diff             @@
##              dev     #192      +/-   ##
==========================================
+ Coverage   95.42%   95.53%   +0.11%     
==========================================
  Files          50       51       +1     
  Lines        2642     2757     +115     
==========================================
+ Hits         2521     2634     +113     
- Misses         93       95       +2     
  Partials       28       28              
Files Changed Coverage Δ
syncx/cond.go 95.65% <95.65%> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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.

总体上来说,这就是之前我说的一个等待者一个 chan 的实现,类似于 sql.DB 这种连接池的设计。核心缺陷就是如果等待者很多,这里会创建很多 chan,倒也可以接受。

@longyue0521 请帮忙看看这边的并发问题。

syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond_test.go Outdated Show resolved Hide resolved
syncx/cond_test.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
@fifth-month
Copy link
Contributor Author

整体上移除了notifyItem的结构,方法打散在了notifyList中。修改了部分测试,增加验证了超时控制的情况下唤醒的有序性

@fifth-month
Copy link
Contributor Author

方法的使用注释加好了,基本上参考了sync.Cond的注释

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.

虽然有点坑,但是我还想说,换成中文写注释,因为我这个首要考虑我们中国人能读懂。翻译软件那么发达,老外自己想用就自己翻译。而且……正常来说中国人的开源项目比较难得到老外的认可,所以不需要考虑英文。你看我们的文档、注释和错误信息全部都是中文……

syncx/cond.go Outdated Show resolved Hide resolved
@flycash
Copy link
Contributor

flycash commented Jun 13, 2023

@fifth-month 我暂时关了这个合并请求,你在企业微信或者邮箱给我发一个邮件,我有点小问题问你一下=。=不好意思了

@flycash flycash closed this Jun 13, 2023
@flycash flycash reopened this Jun 13, 2023
@fifth-month
Copy link
Contributor Author

fifth-month commented Jun 14, 2023

把方法使用注释改成了中文,将WaitWithContext和Wait合并成了Wait(ctx)方法,优化了channel的使用逻辑,使用发送消息,代替关闭channel来做通知,同时由于不再使用关闭channel来通知,使得channel能够被复用,所以加入了sync.Pool优化channel的创建问题,benchmark测试内存分配降低了一半。但是还有一些内存分配主要集中在list的Element的分配上,但这个官方库没有办法复用list.Element,可能得自己写一个双链表才可以解决这个问题。
1686711851696

@longyue0521
Copy link
Collaborator

@fifth-month @flycash

我一直在强调并确认wait(ctx) error的逻辑语义,这是因为wait(ctx) error的后置条件(可以依赖的假设)对用户的使用体验有很大影响。

场景分析:

  1. 当调用者协程因超时返回,且中途没有被notify,wait(ctx) !=nil && ctx.Err() != nil
  2. 当调用者协程因notify返回,且中途没有超时,wait(ctx) == nil && ctx.Err() == nil
  3. 当调用者协程因超时返回,返回途中被notify或者两个信号同时出现因select的随机性选择了超时分支,wait(ctx) == ? && ctx.Err() != nil
  4. 当调用者协程因notify返回,返回途中超时或者两个信号同时出现因select的随机性选择了notify分支, wait(ctx) == ? && ctx.Err() != nil

当前实现在场景3和4中对外的逻辑语义是wait(ctx) == nil && ctx.Err() != nil,作为一个对超时敏感的用户我就很困惑,场景1逻辑语义wait(ctx) !=nil && ctx.Err() != nil 两者返回值相匹配,代码会走入超时逻辑分支if err := wait(ctx); err != nil {,而场景3和4的逻辑语义是wait(ctx) == nil && ctx.Err() != nil两者返回值不匹配,如果我想进入超时逻辑分支我必须if err := wait(ctx); err != nil || ctx.Err() != nil {。接着就出现了_ := wait(ctx); if ctx.Err() != nil { ....}

那么wait(ctx)的返回值到底表示什么?为什么是error类型。

我认为wait(ctx)的返回值表示的是”信号“出现的情况——仅超时信号,仅唤醒信号或协程从wait返回时最终的状态(两个信号都存在)
使用error类型返回值会像上面举例那样”误导“用户,不如限定wait(ctx) (bool)表示是否被唤醒,超时信号由ctx.Err承担,用户想控制超时自己写if ctx.Err() != nil {,当两个信号同时出现时wait(ctx) == true && ctx.Err() != nil,用户可以在两个信号之间自主选择是超时还是继续。

@fifth-month
Copy link
Contributor Author

你说的意思,我懂,语义的问题确实是存在的,但是我个人觉得这个调度机制本身就没办法去保证一定是实时的问题,就算你
select {
case <- ctx.Done:
// 超时了
default:
// 没有超时,但是真的没有超时吗
// 也许刚进入default分支就发生协程切换了,
<-- 这里发生了协程切换
// 切换回来再执行的时候难道再检查一次有没有超时?
}
我个人认为,本身go就不能太精准的控制时间问题,特别是在多线程的情况下
如果我们相信就绪状态的goroutine会很快就被调度执行的话,我们就不用太在意是否恰好超时的问题

@fifth-month
Copy link
Contributor Author

fifth-month commented Jun 14, 2023

不过你说的返回bool确实感觉会更合理点,我觉得这里Wait(ctx) bool返回true代表条件有了变化被唤醒,false代表条件没有变化被唤醒(也就是超时而唤醒的)。 @flycash @longyue0521

@flycash
Copy link
Contributor

flycash commented Jun 14, 2023

其实我还是倾向于保持和其它超时控制一样的语义。就是使用 error。

在正常的使用场景下,我认为大部分用户不会特别关心是什么错误:

if wait(ctx) != nil {
    return //
}

如果要进一步深入下去, if wait(ctx) == context.DeadlineExceed; 就是超时;而 wait(ctx)==context.Cancel 就是被取消了。这个语义是非常清楚的。如果是 bool 的话是分辨不出来这两种情况的。

关于时机的问题,也就是如果要是超时和唤醒同时发生,我觉得这里倒是没必要仔细去分辨。比如说我们在sql.DB 里面也有类似的问题,即获取链接和超时同时发生。他们实际上就是,你进去了 select 哪个分支,就认为是哪个情况。

image

我们这里在超时之后,如果刚好也收到了信号,能不能唤醒下一个人(比如在上图,他们是把连接放回去了)?这样就是永远保证,在超时的时候返回 ctx.Err。这样比较贴近已有的做法。

@longyue0521
Copy link
Collaborator

其实我还是倾向于保持和其它超时控制一样的语义。就是使用 error。

在正常的使用场景下,我认为大部分用户不会特别关心是什么错误:

if wait(ctx) != nil {
    return //
}

如果要进一步深入下去, if wait(ctx) == context.DeadlineExceed; 就是超时;而 wait(ctx)==context.Cancel 就是被取消了。这个语义是非常清楚的。如果是 bool 的话是分辨不出来这两种情况的。

关于时机的问题,也就是如果要是超时和唤醒同时发生,我觉得这里倒是没必要仔细去分辨。比如说我们在sql.DB 里面也有类似的问题,即获取链接和超时同时发生。他们实际上就是,你进去了 select 哪个分支,就认为是哪个情况。

image 我们这里在超时之后,如果刚好也收到了信号,能不能唤醒下一个人(比如在上图,他们是把连接放回去了)?这样就是永远保证,在超时的时候返回 ctx.Err。这样比较贴近已有的做法。

@fifth-month 按照这个语义调整一下你的代码。

@fifth-month
Copy link
Contributor Author

调整好了,现在是超时收到正常信号,会传递给下一个队首.

syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond_test.go Outdated Show resolved Hide resolved
syncx/cond_test.go Outdated Show resolved Hide resolved
@longyue0521
Copy link
Collaborator

@fifth-month 每修改一处后点击resolve conversion将相应对话框关闭。

@longyue0521
Copy link
Collaborator

@fifth-month 你还准备继续这个PR吗?

@fifth-month
Copy link
Contributor Author

应该不急吧,还得等几天

@fifth-month
Copy link
Contributor Author

本来是想实现一个简版的linkedlist,然后配合notifyList使用,但是发现链表元素在移除的时候回收和channel的回收不同步进行,会导致死锁,索性干脆把chan直接挪入linkedlist的实现里面改成了chanList的一个特定实现,只为了这个模块而存在的一个channel的双链表。

syncx/cond.go Show resolved Hide resolved
syncx/cond.go Show resolved Hide resolved
syncx/cond.go Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
syncx/cond.go Outdated Show resolved Hide resolved
@fifth-month
Copy link
Contributor Author

增加了运行时拷贝检测,修正了初始化体验,尽可能保证初始化使用方式和官方Cond一致(支持syncx.Cond{L:sync.Locker})

@flycash flycash merged commit 5f533c9 into ecodeclub:dev Aug 10, 2023
7 checks passed
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