refactor: rewrite the queue to make context aware #896
Conversation
ring.go
Outdated
| n := &r.store[atomic.AddUint32(&r.write, 1)&r.mask] | ||
| n.c1.L.Lock() | ||
| for n.mark != 0 { | ||
| if ctxCh := ctx.Done(); ctxCh != nil { | ||
| select { | ||
| case <-ctxCh: | ||
| n.c1.L.Unlock() | ||
| n.c1.Signal() | ||
| return nil, ctx.Err() | ||
| default: | ||
| } | ||
| } | ||
| n.c1.Wait() |
There was a problem hiding this comment.
Hi @proost, thanks! I think we are close, but 2 things:
- We should find a way to make the condition variable context aware,
n.c1.Wait(ctx), instead of a precheck. A precheck has already been done at the beginning of thepipe.Do(). - Since we did
atomic.AddUint32(&r.write, 1), ther.writehas been incremented, and we may leave the correspondingr.storeslot empty if thectxis errored. Do we handle the case correctly?
There was a problem hiding this comment.
We should find a way to make the condition variable context aware, n.c1.Wait(ctx), instead of a precheck. A precheck has already been done at the beginning of the pipe.Do().
I knew that point. I already said this PR doesn't solve the issue completely. This PR mitigates overhead when buffer is full and some requests are already canceled or timeout. Do you want solve problem completely? If then i need some time. because bigger code change is needed.
There was a problem hiding this comment.
I know you know, but since we have already done a precheck at the beginning of pipe.Do(), a second precheck in the ring doesn't really help much. However, you did a great job on changing the ring interface. 👍
There was a problem hiding this comment.
I know you know, but since we have already done a precheck at the beginning of pipe.Do(), a second precheck in the ring doesn't really help much.
I'm not sure, but issue i mentioned, looks like happening when after pipe.Do(), so it might helpful the situation.
Regardless of second precheckiong, complete solution is needed I think.
There was a problem hiding this comment.
I rewrite the queue.
proost
changed the title
tiny improvement about the issue
This PR doesn't solve above issue. To solve the issue, I think rewriting ring buffer is needed. But That's quite huge.
This PR improves two things.
I think this tiny improvement is helpful for very rare situation, that buffer is full and lots of requests are waiting other requests.