Fix: request counter race for concurrent requests matching the same interaction #46
Conversation
For each interaction, we keep track of the number of times it has been called. That counter can be used in the modifiers to change the response returned to the client. For example it is possible to change the return status on the 2nd invocation of a specific interaction. The code has a bug when concurrent requests happen for the same interaction. The change modifies the way the concurrency is managed just for the request counter, using channels instead of Lock. As part of this change, we have added a new test that can be used to prove that the original code has the issue and the proposed version fixes it. Co-authored-by: Shreya Garge <[email protected]>
andrea-f3
force-pushed
the
sg-ar-concurrency
branch
from
1f5386d
to
1988374
Compare
Co-authored-by: Andrea Rosa <[email protected]>
|
closed #35 as duplicate |
|
|
||
| func (s *ConcurrentProxyStage) the_second_user_response_should_have_a_modified_body() *ConcurrentProxyStage { | ||
| c := 0 | ||
| for _, res := range s.userResponses { |
There was a problem hiding this comment.
could we try to make tests a bit more reliable, for example:
- verify that there is exactly 2 responses beforehand
- verify body and status code of both responses, one of them being modified the other one not
| func (s *ConcurrentProxyStage) the_concurrent_requests_are_sent_with_attempt_based_modifier() { | ||
| err := s.pact.Verify(func() (err error) { | ||
| attempt := 2 | ||
| s.proxy.ForInteraction(postNamePactWithAnyName).AddModifier("$.status", fmt.Sprintf("%d", http.StatusConflict), &attempt) |
There was a problem hiding this comment.
Jjust wondering, is it out of scope to verify adding different modifiers on different attempts within same test?
like on 2nd attempt 409 and body1 on 3rd 418 and body2
not sure there is a use case but would be a valid test to verify interactions are correctly modified, if it is out of scope, feel free to ignore
There was a problem hiding this comment.
@sofib-form3 We are doing this in a different branch. Will raise a PR soon.
Co-authored-by: Andrea Rosa <[email protected]>
We have tested it in a consumer, yes |
If multiple requests matching the same interaction are made concurrently, we have a race condition where the latter requests can update the request counter before the former requests read them, giving the wrong count for the former requests. If we then also have a modifier based on attempt number, the modifier gets wrongly applied to the former requests.
This is elaborated further using this sequence diagram:
sequenceDiagram autonumber participant G as request 1 participant R as Interaction participant A as request 2 note over R: requestcount = 0 activate G note over G: Match Interaction rect rgb(100, 100, 150) Note over G, R: lock interaction G -->> R: requestCount 0 -> 1 Note over G, R: release lock end note over R: requestCount = 1 activate A note over A: Match Interaction rect rgb(100, 100, 150) Note over A, R: lock interaction A -->> R: requestCount 1 -> 2 Note over A, R: release lock end note over R: requestCount = 2 note over G: Modify/Write Response G -->> R: getRequestCount R ->> G: requestCount = 2 deactivate G note over A: Modify/Write Response A -->> R: getRequestCount R ->> A: requestCount = 2 deactivate ATo fix this we create a local copy of matched interactions and request count instead of the interaction at it's address directly