search

joncrlsn / dque

dque is a fast, embedded, durable queue for Go
MIT License
767 stars 46 forks source link

DQue.Size and SegmentNumbers are not thread-safe #8

Closed neilisaac closed 4 years ago

neilisaac commented 4 years ago

First off, thanks for writing this package! I'm doing a brief code review on it to evaluate it for production use, and am filing issues along the way. Please let me know if you'd rather we fork it or submit patches for review.

qSegment.size() is thread-safe, however DQue.Size is not since firstSegment and lastSegment may be updated asynchronously (ex. from Enqueue/Dequeue).

joncrlsn commented 4 years ago

Hi Neil, I welcome pull requests. Thank you for looking at this.

The Enqueue and Dequeue methods are synchronized with a mutex lock, so firstSegement and lastSegment should not be updated asynchronously. Let me know if you think I'm missing something. Thank you in advance for the other questions. I'll look at those next.

neilisaac commented 4 years ago

The Size method is not mutex protected, so the first/last segment pointers could theoretically change in the middle of execution. It would likely be called from a separate goroutine for use cases like reporting metrics about the queue backlog size.

joncrlsn commented 4 years ago

Yeah, I wasn't wanting to lock the mutex just to get the size. The comment tried to warn users that it's not always going to be exact, but I could add a SafeSize() method that locks things up for cases that care about an exact number. Thoughts?

neilisaac commented 4 years ago

I'd generally suggest making the default behavior safe. If needed for some use case, Size and SizeUnsafe seems more intuitive than Size and SafeSize.

Size will only hold the mutex briefly, so readers and writers won't be materially affected, though calls to Size may be delayed by disk IO. That said, locking performance doesn't appear to be a priority considering that readers and writers currently block each other.

neilisaac commented 4 years ago

This block of code in Size() appears to be redundant (and never true since it's backwards):

    if q.firstSegment.number == q.lastSegment.number+1 {
        return q.firstSegment.size() + q.lastSegment.size()
    }
joncrlsn commented 4 years ago

I like Size() and SizeUnsafe(). I think we could do that without breaking the API (the SizeSafe method has not really been in the codebase long enough to consider it part of the API) so we should probably make that change soon. I'll do it now.

Thank you catching that error. I'll fix that too.