feat(transaction-is-ended): add IsEnded() method (#995)

[frknikiz]

Links

• Issue: #995

Details

This pull request introduces a new IsEnded() method for the Transaction type. It addresses the issue raised in #995 where there was no straightforward way to determine if a transaction has ended.

Changes include:

• New Method Implementation:
  The IsEnded() method has been added to check the internal transaction state. It ensures that a transaction is considered ended if it is nil, if its thread is nil, or if the underlying transaction's finished flag is set.

  func (t *Transaction) IsEnded() bool {
      if t == nil || t.thread == nil || t.thread.txn == nil {
          return true
      }
      return t.thread.txn.finished
  }
  

• Unit Tests:

  Unit tests have been added to verify that:

  • A nil Transaction returns true for IsEnded().
  • A Transaction with a nil thread or nil txn returns true.
  • An active transaction returns false.
  • A finished transaction returns true.

These changes ensure robust transaction state checking across different scenarios.
Closes #995.

[CLAassistant]

All committers have signed the CLA.

[iamemilio]

Thanks for the PR, I like the idea. I would like to spend a little time next week to vet that this will be thread safe before moving forward with these changes, but I enabled the automated tests.

[iamemilio]

@frknikiz before we merge this, would you mind making the following changes to follow our style and avoid race conditions:

First, create a method of thread in internal_txn.go

func (thd *thread) IsEnded() bool {
	txn := thd.txn
	txn.Lock()
	defer txn.Unlock()

	return txn.finished
}

Then, invoke that from transaction.go

// IsEnded returns transaction end status.
// If the transaction is nil, the thread is nil, or the transaction is finished, it returns true.
// Otherwise, it returns thread.finished value.
func (txn *Transaction) IsEnded() bool {
	if nilTransaction(txn) {
		return true
	}
	return txn.thread.IsEnded()
}

A final thought, should a nil transaction return true when checking that it has ended? Or should it return an error? I could see this both ways, but I think it would be confusing if someone mistakenly nil'd the pointer but the transaction persisted, and was not finished. When we end transactions, we do not nil out its fields, we allow the garbage collector to handle that. Normally, we don't like to return errors because people want to lean on the value our tool provides without needing to concern themselves with how it provides that value. However, since this is asking to look inside at the state of the tool itself, it does break from that norm, so I think it would be reasonable to return an error so people don't need to guess what they did wrong here. I am curious what you would prefer for your use case.

An alternative solution could look like this:

var ErrorCheckIsEndedOnNil = errors.New("a Transaction must not be `nil` in order to call `IsEnded` on it")

func (txn *Transaction) IsEnded() (bool, error) {
	if nilTransaction(txn) {
		return false, ErrorCheckIsEndedOnNil
	}
	return txn.thread.IsEnded(), nil
}

[iamemilio]

we also introduced a new function you can invoke to check for nil transactions which has just been merged into develop, and will be released this week. I will update the suggestion above to implement it.

// nilTransaction guards against nil errors when handling a transaction.
func nilTransaction(txn *Transaction) bool {
	return txn == nil || txn.thread == nil || txn.thread.txn == nil
}

[frknikiz]

The IsEnded method gives us information about whether the transaction has actually ended or not. For this reason, I think this error should not be returned.

If the transaction is somehow nil,i think it's fine then the transaction is finished.

[iamemilio]

To clarify, the Go Agent will never make a transaction, its data, or a pointer to a transaction nil. So if it is nil, it is likely due to a logical error in the user's code.

The tests look good, thanks for updating this!

[frknikiz]

Thanks for your support 🙏