vmware-archive / go-pmem-transaction

Golang library for using persistent memory
Other
29 stars 5 forks source link

Return error from End() if inner transaction ends #31

Closed mohit10verma closed 5 years ago

mohit10verma commented 5 years ago

And return nil from End() if outer transaction ends. This can be used by a user to identify when is it safe to release the transaction handle. (Only when outer transaction ends & the error returned is nil). Changed only for undoTx. This particular change is used by compiler changes to use txn() for identifying when to release transaction handle Signed-off-by: Mohit Verma mohitv@vmware.com

jerrinsg commented 5 years ago

With this change, the error handling code would become more verbose. How about changing End() to return the current level as well or add a new API in transaction package to check if the handle can be released? E.g.:

With this change:

// something like
err := txn.End()
if err == errors.New("[undoTx] End: no transaction to commit!") {
    // handle error - abort(), etc.
    log.Fatal(err)
}
if err == errors.New("[undoTx] End: Inner transaction. Nothing to do") {
    // release handle
    transaction.Release(txn)
}

Return level:

level, err := txn.End()
if err != nil {
    // handle error - abort(), etc.
    log.Fatal(err)
}
if level == 0 {
    // release handle
    transaction.Release(txn)
}

Add a new API:

err := txn.End()
if err != nil {
    // handle error - abort(), etc.
    log.Fatal(err)
}
if !transaction.IsPending(txn) {
    // release handle
    transaction.Release(txn)
}
mohit10verma commented 5 years ago

I am not a big fan of adding a new API. So, I can do two things:

  1. Return level, error from End() & use as you suggested above.
  2. Further change End() function to return nil if outer transaction completed successfully, error if inner transaction Ends and crash if End() is called before Begin(). With 2nd option, End() would look like this:
func (t *undoTx) End() error {
    if t.level == 0 {
        log.Fatalf() // <-- CRASH INSTEAD OF ERROR
    }
    t.level--
    if t.level == 0 {

        // NO CHANGE

        return nil
    }
    return errors.New("[undoTx] End: Inner transaction. Nothing to do")
}
mohit10verma commented 5 years ago

Closing this. I'll open a new pull request, changing what End() method returns. It should return a bool indicating whether it is safe to release a transaction handle or not.