Skip to content

Commit 07ce579

Browse files
deiwindeiwin
committed
Allow first try to be synchronous
Some operations which would benefit from this delayed retry mechanism may often complete just fine synchronously. This optimization will make the first try synchronously, if so configured. As can be seen from the test changes, this also simplifies the tests quite a bit, because we can now check the responses synchronously in most cases.
1 parent 3dd66bb commit 07ce579

File tree

6 files changed

+68
-62
lines changed

6 files changed

+68
-62
lines changed

‎async.go

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package main
22

33
import (
44
"errors"
5+
"fmt"
56
"log"
67
"os"
78
"os/signal"
@@ -14,7 +15,47 @@ type asyncResponse struct {
1415
MayBeRetried bool
1516
}
1617

18+
// MaybeSyncResponse can be returned from operations which may or may not
19+
// complete synchronously. When OperationFinishedSynchronously is true, then
20+
// Response will be specified.
21+
type MaybeSyncResponse struct {
22+
Response
23+
OperationFinishedSynchronously bool
24+
}
25+
26+
func syncResponse(response Response) MaybeSyncResponse {
27+
return MaybeSyncResponse{
28+
Response: response,
29+
OperationFinishedSynchronously: true,
30+
}
31+
}
32+
1733
func delayWithRetries(tryDelays []time.Duration, operation func() asyncResponse,
34+
asyncOperationWg *sync.WaitGroup) (MaybeSyncResponse, error) {
35+
36+
if len(tryDelays) < 1 {
37+
return MaybeSyncResponse{}, errors.New("Cannot schedule any delayed operations when tryDelays is empty")
38+
}
39+
40+
if tryDelays[0] == 0 {
41+
response := operation()
42+
if len(tryDelays) > 1 && response.MayBeRetried {
43+
log.Println("Operation will be retried")
44+
if err := asyncDelayWithRetries(tryDelays[1:], operation, asyncOperationWg); err != nil {
45+
return MaybeSyncResponse{}, fmt.Errorf("Failed to schedule async retries: %v", err)
46+
}
47+
return MaybeSyncResponse{OperationFinishedSynchronously: false}, nil
48+
}
49+
return syncResponse(response), nil
50+
}
51+
52+
if err := asyncDelayWithRetries(tryDelays, operation, asyncOperationWg); err != nil {
53+
return MaybeSyncResponse{}, fmt.Errorf("Failed to schedule async delay with retries: %v", err)
54+
}
55+
return MaybeSyncResponse{OperationFinishedSynchronously: false}, nil
56+
}
57+
58+
func asyncDelayWithRetries(tryDelays []time.Duration, operation func() asyncResponse,
1859
asyncOperationWg *sync.WaitGroup) error {
1960

2061
if len(tryDelays) < 1 {
@@ -26,7 +67,7 @@ func delayWithRetries(tryDelays []time.Duration, operation func() asyncResponse,
2667
handleAsyncResponse(response.Response)
2768
if len(tryDelays) > 1 && response.MayBeRetried {
2869
log.Println("Operation will be retried")
29-
if err := delayWithRetries(tryDelays[1:], operation, asyncOperationWg); err != nil {
70+
if err := asyncDelayWithRetries(tryDelays[1:], operation, asyncOperationWg); err != nil {
3071
log.Printf("Failed to schedule another try to start in %s\n", tryDelays[1].String())
3172
return
3273
}

‎config.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ var (
1515
accessTokenProperty = gonfigure.NewRequiredEnvProperty("GITHUB_ACCESS_TOKEN")
1616
secretProperty = gonfigure.NewRequiredEnvProperty("GITHUB_SECRET")
1717
// A comma separated list of durations in the format defined in
18-
// time.ParseDuration. E.g. "300ms,1.5h,2h45m".
18+
// time.ParseDuration. E.g. "300ms,1.5h,2h45m". When first duration is 0,
19+
// then GitHub API requests will initially be tried synchronously and only
20+
// the retries will be asynchronous.
1921
githubAPITriesProperty = gonfigure.NewEnvProperty("GITHUB_API_TRIES", "0s,10s,30s,3m")
2022
)
2123

‎github_review_helper_suite_test.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,8 +108,13 @@ var TestWebhookHandler = func(test WebhookTest) bool {
108108

109109
githubAPITryDeltas := make([]time.Duration, numberOfGithubTries)
110110
for i := range githubAPITryDeltas {
111-
// Try every 1ms
112-
githubAPITryDeltas[i] = time.Millisecond
111+
if i == 0 {
112+
// Allow the first try to be synchronous
113+
githubAPITryDeltas[i] = 0
114+
} else {
115+
// Try every 1ms
116+
githubAPITryDeltas[i] = time.Millisecond
117+
}
113118
}
114119
conf = grh.Config{
115120
Secret: "a-secret",

‎main.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,11 +118,13 @@ func handleStatusEvent(body []byte, conf Config, asyncOperationWg *sync.WaitGrou
118118
if err != nil {
119119
return ErrorResponse{err, http.StatusInternalServerError, "Failed to parse the request's body"}
120120
} else if newPullRequestsPossiblyReadyForMerging(statusEvent) {
121-
err = delayWithRetries(conf.GithubAPITryDeltas, func() asyncResponse {
121+
maybeSyncResponse, err := delayWithRetries(conf.GithubAPITryDeltas, func() asyncResponse {
122122
return mergePullRequestsReadyForMerging(statusEvent, gitRepos, search, issues, pullRequests)
123123
}, asyncOperationWg)
124124
if err != nil {
125125
return ErrorResponse{err, http.StatusInternalServerError, "Failed to schedule mergeable PR check"}
126+
} else if maybeSyncResponse.OperationFinishedSynchronously {
127+
return maybeSyncResponse.Response
126128
}
127129
return SuccessResponse{"Status update might have caused a PR to become mergeable. Will check for " +
128130
"mergeable PRs asynchronously"}

‎merge_command_state_update_test.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -103,11 +103,9 @@ var _ = TestWebhookHandler(func(context WebhookTestContext) {
103103
mockSearchQuery(1).Return(emptyResult, emptyResponse, errors.New("arbitrary error"))
104104
})
105105

106-
// async errors are logged, but won't affect the outcome of
107-
// the HTTP request
108-
It("returns 200 OK", func() {
106+
It("fails with a gateway error", func() {
109107
handle()
110-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
108+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
111109
})
112110

113111
It("tries once", func() {
@@ -160,11 +158,9 @@ var _ = TestWebhookHandler(func(context WebhookTestContext) {
160158
Return(emptyResult, emptyResponse, errArbitrary)
161159
})
162160

163-
// async errors are logged, but won't affect the outcome of
164-
// the HTTP request
165-
It("returns 200 OK", func() {
161+
It("fails with a gateway error", func() {
166162
handle()
167-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
163+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
168164
})
169165

170166
It("tries once", func() {
@@ -195,8 +191,7 @@ var _ = TestWebhookHandler(func(context WebhookTestContext) {
195191
Return(pr, emptyResponse, noError)
196192
})
197193

198-
isAsync := true
199-
ItMergesPR(context, pr, isAsync)
194+
ItMergesPR(context, pr)
200195
})
201196
})
202197

‎merge_command_test.go

Lines changed: 8 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ func commentMentioning(user string) func(issueComment *github.IssueComment) bool
218218
}
219219
}
220220

221-
var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts ...bool) {
221+
var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest) {
222222
var (
223223
handle = context.Handle
224224

@@ -230,11 +230,6 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
230230
issueAuthor string
231231
issueNumber int
232232
headRef string
233-
234-
// async errors are logged, but won't affect the outcome of the HTTP
235-
// request. The tests can only confirm that the right stubs were called
236-
// for async operations.
237-
isAsync bool
238233
)
239234
BeforeEach(func() {
240235
responseRecorder = *context.ResponseRecorder
@@ -245,12 +240,6 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
245240
issueAuthor = *pr.User.Login
246241
issueNumber = *pr.Number
247242
headRef = *pr.Head.Ref
248-
249-
if len(opts) == 0 {
250-
isAsync = false
251-
} else {
252-
isAsync = opts[0]
253-
}
254243
})
255244

256245
Context("with merge failing with an unknown error", func() {
@@ -272,11 +261,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
272261

273262
It("fails with a gateway error", func() {
274263
handle()
275-
if isAsync {
276-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
277-
} else {
278-
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
279-
}
264+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
280265
})
281266
})
282267

@@ -323,11 +308,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
323308
Return(emptyResult, emptyResponse, noError)
324309

325310
handle()
326-
if isAsync {
327-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
328-
} else {
329-
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
330-
}
311+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
331312
})
332313

333314
Context("with author notification failing", func() {
@@ -340,11 +321,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
340321

341322
It("fails with a gateway error", func() {
342323
handle()
343-
if isAsync {
344-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
345-
} else {
346-
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
347-
}
324+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
348325
})
349326
})
350327
})
@@ -392,11 +369,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
392369

393370
It("fails with a gateway error", func() {
394371
handle()
395-
if isAsync {
396-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
397-
} else {
398-
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
399-
}
372+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
400373
})
401374
})
402375

@@ -428,11 +401,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
428401

429402
It("fails with a gateway error", func() {
430403
handle()
431-
if isAsync {
432-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
433-
} else {
434-
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
435-
}
404+
Expect(responseRecorder.Code).To(Equal(http.StatusBadGateway))
436405
})
437406
})
438407

@@ -453,11 +422,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
453422

454423
It("fails with an internal error", func() {
455424
handle()
456-
if isAsync {
457-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
458-
} else {
459-
Expect(responseRecorder.Code).To(Equal(http.StatusInternalServerError))
460-
}
425+
Expect(responseRecorder.Code).To(Equal(http.StatusInternalServerError))
461426
})
462427
})
463428

@@ -478,11 +443,7 @@ var ItMergesPR = func(context WebhookTestContext, pr *github.PullRequest, opts .
478443

479444
It("fails with an internal error", func() {
480445
handle()
481-
if isAsync {
482-
Expect(responseRecorder.Code).To(Equal(http.StatusOK))
483-
} else {
484-
Expect(responseRecorder.Code).To(Equal(http.StatusInternalServerError))
485-
}
446+
Expect(responseRecorder.Code).To(Equal(http.StatusInternalServerError))
486447
})
487448
})
488449

0 commit comments

Comments
 (0)