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

Panic when trying to use uninitialized or deleted Pigosat objects #6

Merged
merged 4 commits into from
Feb 13, 2015

Conversation

wkschwartz
Copy link
Owner

Based on justinfx/pigosat@a755660. Open to suggestions about how to test 35d2c7f.

Similar to @justinfx/pigosata755660
but use simpler syntax in isReady function (boolean expressions return a boolean
value) and call it isReady rather than isValid because there's already the
NotReady status constant.
I can't think of a good way to test this. Open to suggestions.
@wkschwartz
Copy link
Owner Author

35d2c7f is wrong. The whole point of p.isReady() is to check whether p is nil, and thus if the lock can be accessed. Perhaps we should lock inside p.isReady(), have one for reading and one for read/write.

func (p *Pigosat) readyRead() (unlock func()) {
    // No race here because the pointer p cannot magically change if it's nil.
    if p == nil {
        return nil
    }
    p.lock.RLock()
    if p.p == nil {
        p.lock.RUnlock()
        return nil
    }
    return p.lock.RUnlock
}

Then it's used like

func (p *Pigosat) Print(file *os.File) error {
    unlock := readyRead()
    if unlock == nil {
        return nil
    }
    defer unlock()
    // ...
}

(Except for extra calls to Delete, which is a no op). This is much simpler than
returning errors for what is a pretty difficult programming error to make. The
prior behavior of returning a variety of zero values just wasn't helpful for
debugging.
@wkschwartz
Copy link
Owner Author

The real issue is that having default return values for when p is nil or already deleted just doesn't make sense. Better to panic for such a hard-to-achieve programming error.

@wkschwartz wkschwartz changed the title Factor out is-nil method guard Panic when trying to use uninitialized or deleted Pigosat objects Feb 13, 2015
@wkschwartz wkschwartz merged commit 365d374 into master Feb 13, 2015
@wkschwartz wkschwartz deleted the isready branch February 13, 2015 06:05
@wkschwartz wkschwartz modified the milestone: v1.0 beta Jan 5, 2017
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.

1 participant