Skip to content

Commit

Permalink
Fix execution of failpoint should not block deactivation
Browse files Browse the repository at this point in the history
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 `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:
- #64

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
  • Loading branch information
henrybear327 committed Apr 18, 2024
1 parent 93c579a commit 361aac9
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 4 deletions.
16 changes: 12 additions & 4 deletions runtime/failpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,13 @@ package runtime

import (
"fmt"
"sync"
)

type Failpoint struct {
t *terms

failpointMu sync.RWMutex
}

func NewFailpoint(name string) *Failpoint {
Expand All @@ -28,14 +31,19 @@ func NewFailpoint(name string) *Failpoint {

// Acquire gets evalutes the failpoint terms; if the failpoint
// is active, it will return a value. Otherwise, returns a non-nil error.
//
// Notice that during the exection of Acquire(), the failpoint can be disabled,
// but the already in-flight execution won't be terminated
func (fp *Failpoint) Acquire() (interface{}, error) {
failpointsMu.RLock()
defer failpointsMu.RUnlock()
fp.failpointMu.RLock()
// terms are locked during execution, so deepcopy is not required as no change can be made during execution
cachedT := fp.t
fp.failpointMu.RUnlock()

if fp.t == nil {
if cachedT == nil {
return nil, ErrDisabled
}
result := fp.t.eval()
result := cachedT.eval()
if result == nil {
return nil, ErrDisabled
}
Expand Down
11 changes: 11 additions & 0 deletions runtime/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ func enable(name, inTerms string) error {
fmt.Printf("failed to enable \"%s=%s\" (%v)\n", name, inTerms, err)
return err
}

fp.failpointMu.Lock()
defer fp.failpointMu.Unlock()

fp.t = t

return nil
Expand All @@ -104,6 +108,9 @@ func disable(name string) error {
return ErrNoExist
}

fp.failpointMu.Lock()
defer fp.failpointMu.Unlock()

if fp.t == nil {
return ErrDisabled
}
Expand All @@ -116,6 +123,7 @@ func disable(name string) error {
func Status(failpath string) (string, int, error) {
failpointsMu.Lock()
defer failpointsMu.Unlock()

return status(failpath)
}

Expand All @@ -125,6 +133,9 @@ func status(failpath string) (string, int, error) {
return "", 0, ErrNoExist
}

fp.failpointMu.RLock()
defer fp.failpointMu.RUnlock()

t := fp.t
if t == nil {
return "", 0, ErrDisabled
Expand Down

0 comments on commit 361aac9

Please sign in to comment.