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

fix blocking Depth() call on the closed queue #12

Merged
merged 2 commits into from
Feb 28, 2020
Merged

Conversation

mgurevin
Copy link
Contributor

No description provided.

@ploxiln
Copy link
Member

ploxiln commented Feb 19, 2020

Looks like a fixup for #11, thanks.

@ploxiln
Copy link
Member

ploxiln commented Feb 19, 2020

In what program did you observe this problem, btw? Was it nsqd or something else?

@mgurevin
Copy link
Contributor Author

I'm using this package in my program to have disk backed channels.

I think the depth method would be better if it returns the real value from the metadata file when the queue is closed. Maybe the last value of the depth before the queue closing can also be used.

@ploxiln
Copy link
Member

ploxiln commented Feb 20, 2020

Yeah ... maybe something like this, in addition to your fix, would be sufficient:

--- a/diskqueue.go
+++ b/diskqueue.go
@@ -138,7 +138,12 @@ func New(name string, dataPath string, maxBytesPerFile int64,
 
 // Depth returns the depth of the queue
 func (d *diskQueue) Depth() int64 {
-       return <-d.depthChan
+       depth, ok := <-d.depthChan
+       if !ok {
+               // ioLoop exited
+               depth = d.depth
+       }
+       return depth
 }

@mgurevin
Copy link
Contributor Author

That was my first attempt too. I prefer this if it's also ok for you.

@ploxiln
Copy link
Member

ploxiln commented Feb 20, 2020

Yeah that seems fine to me. You want to add that bit to this PR?

instead of 0 (or, before the preceding change, blocking indefinitely)
@ploxiln ploxiln merged commit 5d0854e into nsqio:master Feb 28, 2020
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.

2 participants