Skip to content

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

Merged
wkschwartz merged 4 commits intomasterfrom
isready
Feb 13, 2015
Merged

Panic when trying to use uninitialized or deleted Pigosat objects#6
wkschwartz merged 4 commits intomasterfrom
isready

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