Fix execution of failpoint should not block deactivation

[henrybear327]

There are 2 main flows of the gofail library: namely, enable/disable and execution (Acquire) of the failpoints.

Currently, a mutex is protecting both flows, thus, only one action can make progress at a time.

This PR proposes a fine-grained mutex, as each failpoint is protected under a dedicated RWMutex. The existing failpointsMu will only be protecting the main shared data structures, such as the failpoints map.

Notice that in our current implementation, the execution of the same failpoint is still sequential (there is a lock within eval on the term being executed)

Reference:

• Execution of failpoint should not block deactivation #64

[henrybear327]

I made a small demo program to demonstrate the effect after applying this change.

[henrybear327]

cc: @ahrtr @serathius

[ahrtr]

Please read

gofail/runtime/http.go

Lines 40 to 45 in 93c579a

	// This prevents all failpoints from being triggered. It ensures
	// the server(runtime) doesn't panic due to any failpoints during
	// processing the HTTP request.
	// It may be inefficient, but correctness is more important than
	// efficiency. Usually users will not enable too many failpoints
	// at a time, so it (the efficiency) isn't a problem.

Overall, I agree that enable/disable and execution (Acquire) of the failpoints should have separate locks, but the failpoint panic is a special case. When the http server is processing request, we should block the execution of panic failpoint; otherwise it may affect the client side of the http request.

gofail/runtime/terms.go

Lines 284 to 291 in 93c579a

	var actMap = map[string]actFunc{
	"off": actOff,
	"return": actReturn,
	"sleep": actSleep,
	"panic": actPanic,
	"break": actBreak,
	"print": actPrint,
	}

[henrybear327]

Please read

gofail/runtime/http.go

Lines 40 to 45 in 93c579a

	// This prevents all failpoints from being triggered. It ensures
	// the server(runtime) doesn't panic due to any failpoints during
	// processing the HTTP request.
	// It may be inefficient, but correctness is more important than
	// efficiency. Usually users will not enable too many failpoints
	// at a time, so it (the efficiency) isn't a problem.

Overall, I agree that enable/disable and execution (Acquire) of the failpoints should have separate locks, but the failpoint panic is a special case. When the http server is processing request, we should block the execution of panic failpoint; otherwise it may affect the client side of the http request.

gofail/runtime/terms.go

Lines 284 to 291 in 93c579a

	var actMap = map[string]actFunc{
	"off": actOff,
	"return": actReturn,
	"sleep": actSleep,
	"panic": actPanic,
	"break": actBreak,
	"print": actPrint,
	}

Hey @ahrtr,

Thanks for reviewing and pointing out this issue.

Sorry that it took a while to fix this. I didn't know if there exists a cleaner way to implement this.

Currently, I introduced a special mutex called panicMu, which ensures that panic failpoints and serving of the HTTP requests won't be able to be executed at the same time.

/cc @ahrtr

[henrybear327]

/cc @serathius

[ahrtr]

Overall looks good to me.

Have you manually verified that it can resolve the issue #64?

[ahrtr]

@ArkaSaha30, please let me know if you have any comments.

[henrybear327]

Overall looks good to me.

Have you manually verified that it can resolve the issue #64?

Hi @ahrtr,

#65 (comment)

I used this small program to perform testing :) Maybe I should add an integration test to make sure that there isn't any regression. :)

[serathius]

Agree about the integration test, it should be enough to test that we can setup a failpoint with 1 second sleep and deactivate it before 1 second passes.

[henrybear327]

Agree about the integration test, it should be enough to test that we can setup a failpoint with 1 second sleep and deactivate it before 1 second passes.

That's similar to what I have in the demo program! :)

I will implement this in the integration test! Thanks @serathius

[ahrtr]

I will implement this in the integration test!

PLease let me know if you want to do it in this PR or in a separate PR.

[henrybear327]

@ahrtr let me try to add the integration test in this PR, so we can have the code and the test merged together.

What do you think :)

[ahrtr]

Great work!

Thanks @henrybear327

Please feel free to add e2e or integration test in a separate PR.

[henrybear327]

Great work!

Thanks @henrybear327

Please feel free to add e2e or integration test in a separate PR.

I have started another PR to temporarily park my integration test implementation (#69).

@ahrtr can you review and see if we need to have a separate discussion regarding the folder/code structure of the integration test, or I can just use that commit here and we merge everything in one go! :)

Thanks!

[ahrtr]

I have started another PR to temporarily park my integration test implementation (#69).

@ahrtr can you review and see if we need to have a separate discussion regarding the folder/code structure of the integration test, or I can just use that commit here and we merge everything in one go! :)

I was only expecting your to do manual test, because it needs big effort to setup the e2e or integration test. But big thanks for offering the e2e/integration test. My proposal:

• Resolve the minor comment raised by @serathius
• Manually verify this PR (most likely you already did, but please double confirm);
• Merge this PR;
• Feel free to add e2e test (Add integration tests for gofail #69) in a separate PR

After all of above is done, we will release v0.2.0.

[henrybear327]

I have started another PR to temporarily park my integration test implementation (#69).
@ahrtr can you review and see if we need to have a separate discussion regarding the folder/code structure of the integration test, or I can just use that commit here and we merge everything in one go! :)

I was only expecting your to do manual test, because it needs big effort to setup the e2e or integration test. But big thanks for offering the e2e/integration test. My proposal:

• Resolve the minor comment raised by @serathius
• Manually verify this PR (most likely you already did, but please double confirm);
• Merge this PR;
• Feel free to add e2e test ([DO-NOT-MERGE] Fix execution of failpoint should not block deactivation integration test #69) in a separate PR

After all of above is done, we will release v0.2.0.

Thanks for the action items @ahrtr, I will push out changes ASAP.

I would add one more step in between. I am also using etcd-io/etcd#18018 to double-check if the gofail changes are working as intended!

[henrybear327]

I have started another PR to temporarily park my integration test implementation (#69).
@ahrtr can you review and see if we need to have a separate discussion regarding the folder/code structure of the integration test, or I can just use that commit here and we merge everything in one go! :)

I was only expecting your to do manual test, because it needs big effort to setup the e2e or integration test. But big thanks for offering the e2e/integration test. My proposal:

• Resolve the minor comment raised by @serathius
• Manually verify this PR (most likely you already did, but please double confirm);
• Merge this PR;
• Feel free to add e2e test ([DO-NOT-MERGE] Fix execution of failpoint should not block deactivation integration test #69) in a separate PR

After all of above is done, we will release v0.2.0.

Thanks for the action items @ahrtr, I will push out changes ASAP.

I would add one more step in between. I am also using etcd-io/etcd#18018 to double-check if the gofail changes are working as intended!

Hi @ahrtr and @serathius,

I have renamed the mutex as suggested, and added a comment to the new mutex :)

I also created etcd-io/etcd#18018, which

• replaces all go.etcd.io/gofail with github.com/henrybear327/gofail
• rolled back the changes that were required due to gofail's original behavior (enabling/disabling failpoints will block the execution of failpoints)
  All the tests can pass (besides the static analysis test failed, because the PR is not using the package from go.etcd.io), which I think means that our current gofail changes aren't breaking the intended functionalities.