367: Fix deferred rollback on error. #391

telackey · 2023-05-31T18:56:04Z

telackey commented

2023-05-31 18:56:04 +00:00

Related to https://github.com/cerc-io/go-ethereum/issues/367

The problem is that the values of the variables in the closure will be evaluated at the moment of the defer statement, not when the function is executed. In these cases, that guarantees that err will be nil, because we would have returned already otherwise.

The fix is to pass in a pointer to the error instead.

With that said, I do wonder if we need such complicated logic here. It would be a lot less complicated and cleaner to do something like:

	base, err := tx.db.Begin(ctx)
	if err != nil {
		return err
	}
	defer base.Rollback()

        // do stuff...
        return base.Commit()

However, that doesn't deal with the panic case nor the additional logging of the code we have, so I didn't change it. Personally, I would lean toward simplicity in error handling unless we are certain we need the complicated code though, precisely to avoid issues like this one.

Related to https://github.com/cerc-io/go-ethereum/issues/367 The problem is that the values of the variables in the closure will be evaluated at the moment of the `defer` statement, not when the function is executed. In these cases, that guarantees that `err` will be nil, because we would have returned already otherwise. The fix is to pass in a pointer to the error instead. With that said, I do wonder if we need such complicated logic here. It would be a lot less complicated and cleaner to do something like: ``` base, err := tx.db.Begin(ctx) if err != nil { return err } defer base.Rollback() // do stuff... return base.Commit() ``` However, that doesn't deal with the panic case nor the additional logging of the code we have, so I didn't change it. Personally, I would lean toward simplicity in error handling unless we are certain we need the complicated code though, precisely to avoid issues like this one.

dboreham reviewed 2023-05-31 18:56:04 +00:00

telackey commented

2023-05-31 19:13:53 +00:00

It is worth adding that I ran this code overnight in the fixturenet along with some intentionally broken logic that generates duplicate ChainEvent every few blocks. It ultimately generated about 3,000 errors, but stayed up and statediffing.

i-norden reviewed 2023-05-31 19:19:40 +00:00

i-norden left a comment

Only variables passed to deferred functions as parameters are evaluated at the time that the defer is called. If instead it is not passed by parameter but is a variable in the scope of the function calling the defer, then the variable is evaluated when the deferred function is executed not when defer is called.

https://go.dev/play/p/sYbwGE_2ay-

So I think the thing that is actually fixing the bug here is that we stop overshadowing the err variable in a number of places, such as on https://github.com/cerc-io/go-ethereum/pull/391/files#diff-9aaf9fbe34f5adee1f41264c3457450e39afd9df2faeda20e49f5b25bcfa3f23R87 and https://github.com/cerc-io/go-ethereum/pull/391/files#diff-b51e10084bd55533c672872e492d79f8733e78febfca935f57123796253538daR850. And I think this would fix the bug without needing to pass a pointer argument to the deffered function (leave it without args) which would look a lot cleaner imo.

I agree it is overcomplicated in either case, open to simplifying it by not catching panics and letting some error messages get overshadowed and unlogged.

Only variables passed to deferred functions as parameters are evaluated at the time that the `defer` is called. If instead it is not passed by parameter but is a variable in the scope of the function calling the defer, then the variable is evaluated when the deferred function is executed not when `defer` is called. https://go.dev/play/p/sYbwGE_2ay- So I think the thing that is actually fixing the bug here is that we stop overshadowing the `err` variable in a number of places, such as on https://github.com/cerc-io/go-ethereum/pull/391/files#diff-9aaf9fbe34f5adee1f41264c3457450e39afd9df2faeda20e49f5b25bcfa3f23R87 and https://github.com/cerc-io/go-ethereum/pull/391/files#diff-b51e10084bd55533c672872e492d79f8733e78febfca935f57123796253538daR850. And I _think_ this would fix the bug without needing to pass a pointer argument to the deffered function (leave it without args) which would look a lot cleaner imo. I agree it is overcomplicated in either case, open to simplifying it by not catching panics and letting some error messages get overshadowed and unlogged.

telackey commented

2023-05-31 20:52:18 +00:00

Corrected. My initial understanding was as you mentioned (per https://go.dev/ref/spec#Defer_statements), and I just switched to = so the variables weren't masked, but stepping through the code I kept seeing the issue until switching to the pointer so concluded that perhaps my understanding was wrong. I think it must have been a build issue though, and I was using an old container, as I fixed it this time, and stepped through the code again, and all is working as expected.

Corrected. My initial understanding was as you mentioned (per https://go.dev/ref/spec#Defer_statements), and I just switched to `=` so the variables weren't masked, but stepping through the code I kept seeing the issue until switching to the pointer so concluded that perhaps my understanding was wrong. I think it must have been a build issue though, and I was using an old container, as I fixed it this time, and stepped through the code again, and all is working as expected.

i-norden approved these changes 2023-05-31 22:31:51 +00:00

i-norden left a comment

LGTM! Looks like this bug is also present in ipld-eth-server where we use a similar defer, will copy this fix over there.

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

3 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cerc-io/go-ethereum#391