Skip to content

Commit 9b2ed24

Browse files
authored
Improve Continuous integration and fix concurrency bugs (#66)
- improvements to the continuous GH actions - fix edge case concurrency bugs with Process.start() and state transitions discovered setting up CI.
1 parent eeb7229 commit 9b2ed24

File tree

3 files changed

+31
-8
lines changed

3 files changed

+31
-8
lines changed

.github/workflows/go-ci.yml

+9-4
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,11 @@
11
# This workflow will build a golang project
22

3-
name: Go
3+
name: CI
44

55
on:
66
push:
77
branches: [ "main" ]
8+
89
pull_request:
910
branches: [ "main" ]
1011

@@ -13,7 +14,7 @@ on:
1314

1415
jobs:
1516

16-
build:
17+
run-tests:
1718
runs-on: ubuntu-latest
1819
steps:
1920
- uses: actions/checkout@v4
@@ -23,5 +24,9 @@ jobs:
2324
with:
2425
go-version: '1.23'
2526

26-
- name: Test
27-
run: make test-all
27+
# necessary for testing proxy/Process swapping
28+
- name: Create simple-responder
29+
run: make simple-responder
30+
31+
- name: Test all
32+
run: make test-all

proxy/process.go

+15
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,24 @@ func (p *Process) start() error {
133133
return nil
134134
}
135135

136+
// There is the possibility of a hard to replicate race condition where
137+
// curState *WAS* StateStopped but by the time we get to the p.stateMutex.Lock()
138+
// below, it's value has changed!
139+
136140
p.stateMutex.Lock()
137141
defer p.stateMutex.Unlock()
138142

143+
// with the exclusive lock, check if p.state is StateStopped, which is the only valid state
144+
// to transition from to StateReady
145+
146+
if p.state != StateStopped {
147+
if p.state == StateReady {
148+
return nil
149+
} else {
150+
return fmt.Errorf("start() can not proceed expected StateReady but process is in %v", p.state)
151+
}
152+
}
153+
139154
if err := p.setState(StateStarting); err != nil {
140155
return err
141156
}

proxy/process_test.go

+7-4
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,8 @@ func TestProcess_LowTTLValue(t *testing.T) {
169169
}
170170

171171
// issue #19
172+
// This test makes sure using Process.Stop() does not affect pending HTTP
173+
// requests. All HTTP requests in this test should complete successfully.
172174
func TestProcess_HTTPRequestsHaveTimeToFinish(t *testing.T) {
173175
if testing.Short() {
174176
t.Skip("skipping slow test")
@@ -192,8 +194,9 @@ func TestProcess_HTTPRequestsHaveTimeToFinish(t *testing.T) {
192194
wg.Add(1)
193195
go func(key string) {
194196
defer wg.Done()
195-
// send a request that should take 5 * 200ms (1 second) to complete
196-
req := httptest.NewRequest("GET", fmt.Sprintf("/slow-respond?echo=%s&delay=200ms", key), nil)
197+
// send a request where simple-responder is will wait 300ms before responding
198+
// this will simulate an in-progress request.
199+
req := httptest.NewRequest("GET", fmt.Sprintf("/slow-respond?echo=%s&delay=300ms", key), nil)
197200
w := httptest.NewRecorder()
198201

199202
process.ProxyRequest(w, req)
@@ -209,9 +212,9 @@ func TestProcess_HTTPRequestsHaveTimeToFinish(t *testing.T) {
209212
}(key)
210213
}
211214

212-
// stop the requests in the middle
215+
// Stop the process while requests are still being processed
213216
go func() {
214-
<-time.After(500 * time.Millisecond)
217+
<-time.After(150 * time.Millisecond)
215218
process.Stop()
216219
}()
217220

0 commit comments

Comments
 (0)