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

Avoid __badType label being executed when the failpoint execution flow is not diverted and the type conversion is successful #66

Merged
merged 4 commits into from
May 14, 2024

Conversation

henrybear327
Copy link
Contributor

Revive #19 (as the PR is closed due to the source repo being deleted by the author, I can only reproduce the commits again)

If the code execution flow after the failpoint type conversion doesn't jump to other parts of the code, the __badType will be executed, causing irrelevant error messages to be printed.

For example, consider the following snippet

// gofail: var SomeFuncString string
// log.Println("SomeFuncString", SomeFuncString)

It will be generated into the following snippet

if vSomeFuncString, __fpErr := __fp_SomeFuncString.Acquire(); __fpErr == nil {
	SomeFuncString, __fpTypeOK := vSomeFuncString.(string)
	if !__fpTypeOK {
		goto __badTypeSomeFuncString
	}
	log.Println("SomeFuncString", SomeFuncString)
__badTypeSomeFuncString:
	__fp_SomeFuncString.BadType(vSomeFuncString, "string")
}

As you can see, because log.println doesn't jump to other parts of the code, after printing the log, the code will continue to execute __badTypeSomeFuncString, thus, printing the following message: "SomeFuncString" got value Hello_world of type "string" but expected type "string"

The solution is to add a __nomock label, in the case of type conversion succeeded, thus the __badType label won't be executed.

Reference:

@ArkaSaha30
Copy link
Contributor

ArkaSaha30 commented Apr 30, 2024

Thank you @henrybear327 for the PR, I will be reviewing it shortly.

@ArkaSaha30
Copy link
Contributor

/assign

@henrybear327
Copy link
Contributor Author

Thank you @henrybear327 for the PR, I will be reviewing it shortly.

Gentle ping :)

I have also rebased it against the latest commit!

@ArkaSaha30
Copy link
Contributor

Hey @henrybear327 👋
I am wondering if we need to update the README.md as well with the __nomock refs

due to the source repo being deleted by the author, I can only reproduce
the commits again)

If the code execution flow after the failpoint type conversion doesn't
jump to other parts of the code, the `__badType` will be executed,
causing irrelevant error messages to be printed.

For example, consider the following snippet
```go
// gofail: var SomeFuncString string
// log.Println("SomeFuncString", SomeFuncString)
```

It will be generated into the following snippet
```go
if vSomeFuncString, __fpErr := __fp_SomeFuncString.Acquire(); __fpErr == nil {
	SomeFuncString, __fpTypeOK := vSomeFuncString.(string)
	if !__fpTypeOK {
		goto __badTypeSomeFuncString
	}
	log.Println("SomeFuncString", SomeFuncString)
__badTypeSomeFuncString:
	__fp_SomeFuncString.BadType(vSomeFuncString, "string")
}
```

As you can see, because `log.println` doesn't jump to other parts of the
code, after printing the log, the code will continue to execute
`__badTypeSomeFuncString`, thus, printing the following message:
`"SomeFuncString" got value Hello_world of type "string" but expected type "string"`

The solution is to add a `__nomock` label, in the case of type
conversion succeeded, thus the `__badType` label won't be executed.

Reference:
- demo code https://github.com/henrybear327/gofail-perf-fix-demo/tree/issue/val_error

Signed-off-by: Chun-Hung Tseng <[email protected]>
@henrybear327 henrybear327 force-pushed the fix/bad_type_issues branch from e82b260 to eab9819 Compare May 10, 2024 14:24
@henrybear327
Copy link
Contributor Author

Hey @henrybear327 👋 I am wondering if we need to update the README.md as well with the __nomock refs

Thanks @ArkaSaha30 for spotting this! Updated as requested!

@ArkaSaha30
Copy link
Contributor

Thank you @henrybear327 .
The changes look good, cc @ahrtr
/lgtm

@ahrtr
Copy link
Member

ahrtr commented May 13, 2024

Thanks for resurrect the PR #19, please also update the examples in https://github.com/etcd-io/gofail/blob/master/doc/design.md#generated-code

@ahrtr
Copy link
Member

ahrtr commented May 13, 2024

It would be great if you can add a test in https://github.com/etcd-io/gofail/blob/master/code/rewrite_test.go to verify the generated code is expected.

@henrybear327
Copy link
Contributor Author

Thanks for resurrect the PR #19, please also update the examples in https://github.com/etcd-io/gofail/blob/master/doc/design.md#generated-code

The generated code is updated! :) Thanks @ahrtr for spotting this missed update!

@henrybear327 henrybear327 force-pushed the fix/bad_type_issues branch from ad2e0dd to 5f671c7 Compare May 13, 2024 21:25
@henrybear327
Copy link
Contributor Author

It would be great if you can add a test in https://github.com/etcd-io/gofail/blob/master/code/rewrite_test.go to verify the generated code is expected.

@ahrtr I manually added the generated output to rewrite_test.go, and checked it :)

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work!

Thanks @henrybear327

@ahrtr ahrtr merged commit 8ce909e into etcd-io:master May 14, 2024
3 checks passed
@henrybear327 henrybear327 deleted the fix/bad_type_issues branch May 14, 2024 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants