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

Race in queue code #22145

Closed
KN4CK3R opened this issue Dec 15, 2022 · 3 comments · Fixed by #22146
Closed

Race in queue code #22145

KN4CK3R opened this issue Dec 15, 2022 · 3 comments · Fixed by #22146
Labels

Comments

@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 15, 2022

Description

Looks like there is still a race in the queue code:

2022/12/14 10:12:18 .../queue/workerpool.go:552:doWork() [E] [6399a182-6] Unhandled Data in queue 3
--- FAIL: TestChannelQueue_Pause (0.60s)
    queue_channel_test.go:281: 
        	Error Trace:	/drone/src/modules/queue/queue_channel_test.go:281
        	Error:      	handler chan should contain test1
        	Test:       	TestChannelQueue_Pause
2022/12/14 10:12:19 ...ueue_channel_test.go:169:func3() [I] Finally terminating
2022/12/14 10:12:19 ...eue/queue_channel.go:153:func1() [W] ChannelQueue:  Terminated before completed flushing
2022/12/14 10:12:20 ...ueue_disk_channel.go:184:func3() [W] [6399a184] LevelQueue: second-level shut down before completely flushed
2022/12/14 10:12:20 ...eue/queue_channel.go:153:func1() [W] ChannelQueue: second-channel Terminated before completed flushing
2022/12/14 10:12:21 ...disk_channel_test.go:202:func1() [I] [6399a184-3] pausing
2022/12/14 10:12:21 ...eue/queue_channel.go:153:func1() [W] ChannelQueue: first-channel Terminated before completed flushing
2022/12/14 10:12:22 ...disk_channel_test.go:202:func1() [I] [6399a186] pausing
2022/12/14 10:12:22 ...eue/queue_channel.go:153:func1() [W] ChannelQueue: second-channel Terminated before completed flushing
2022/12/14 10:12:22 ...ueue_disk_channel.go:184:func3() [W] [6399a185-2] LevelQueue: second-level shut down before completely flushed
2022/12/14 10:12:22 ...disk_channel_test.go:257:func3() [I] Finally terminating

Gitea Version

main

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

https://drone.gitea.io/go-gitea/gitea/64466/2/14

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

CI

Database

None

@KN4CK3R
Copy link
Member Author

KN4CK3R commented Dec 15, 2022

@zeripath fyi

@zeripath
Copy link
Contributor

I think that's more likely a race in the test. Not the code itself.

zeripath added a commit to zeripath/gitea that referenced this issue Dec 15, 2022
There are a few places in FlushQueueWithContext which make an
incorrect assumption about how `select` on multiple channels works.

The problem is best expressed by looking at the following example:

```go
package main

import "fmt"

func main() {
    closedChan := make(chan struct{})
    close(closedChan)
    toClose := make(chan struct{})
    count := 0

    for {
        select {
        case <-closedChan:
            count++
            fmt.Println(count)
            if count == 2 {
                close(toClose)
            }
        case <-toClose:
            return
        }
    }
}
```

This PR double-checks that the contexts are closed outside of checking
if there is data in the dataChan. It also rationalises the WorkerPool
FlushWithContext because the previous implementation failed to handle
pausing correctly. This will probably fix the underlying problem in
 go-gitea#22145

Fix go-gitea#22145

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor

It would also fix the test error here: https://drone.gitea.io/go-gitea/gitea/64883/2/13

lafriks pushed a commit that referenced this issue Dec 30, 2022
There are a few places in FlushQueueWithContext which make an incorrect
assumption about how `select` on multiple channels works.

The problem is best expressed by looking at the following example:

```go
package main

import "fmt"

func main() {
    closedChan := make(chan struct{})
    close(closedChan)
    toClose := make(chan struct{})
    count := 0

    for {
        select {
        case <-closedChan:
            count++
            fmt.Println(count)
            if count == 2 {
                close(toClose)
            }
        case <-toClose:
            return
        }
    }
}
```

This PR double-checks that the contexts are closed outside of checking
if there is data in the dataChan. It also rationalises the WorkerPool
FlushWithContext because the previous implementation failed to handle
pausing correctly. This will probably fix the underlying problem in
 #22145

Fix #22145

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit to zeripath/gitea that referenced this issue Jan 13, 2023
Backport go-gitea#22146

There are a few places in FlushQueueWithContext which make an incorrect
assumption about how `select` on multiple channels works.

The problem is best expressed by looking at the following example:

```go
package main

import "fmt"

func main() {
    closedChan := make(chan struct{})
    close(closedChan)
    toClose := make(chan struct{})
    count := 0

    for {
        select {
        case <-closedChan:
            count++
            fmt.Println(count)
            if count == 2 {
                close(toClose)
            }
        case <-toClose:
            return
        }
    }
}
```

This PR double-checks that the contexts are closed outside of checking
if there is data in the dataChan. It also rationalises the WorkerPool
FlushWithContext because the previous implementation failed to handle
pausing correctly. This will probably fix the underlying problem in
 go-gitea#22145

Fix go-gitea#22145

Signed-off-by: Andrew Thornton <art27@cantab.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this issue Jan 13, 2023
Backport #22146

There are a few places in FlushQueueWithContext which make an incorrect
assumption about how `select` on multiple channels works.

The problem is best expressed by looking at the following example:

```go
package main

import "fmt"

func main() {
    closedChan := make(chan struct{})
    close(closedChan)
    toClose := make(chan struct{})
    count := 0

    for {
        select {
        case <-closedChan:
            count++
            fmt.Println(count)
            if count == 2 {
                close(toClose)
            }
        case <-toClose:
            return
        }
    }
}
```

This PR double-checks that the contexts are closed outside of checking
if there is data in the dataChan. It also rationalises the WorkerPool
FlushWithContext because the previous implementation failed to handle
pausing correctly. This will probably fix the underlying problem in
 #22145

Fix #22145

Signed-off-by: Andrew Thornton <art27@cantab.net>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants