WebHook : UseFireAndForget + Delay #803
Conversation
Codecov Report
@@ Coverage Diff @@
## master #803 +/- ##
==========================================
- Coverage 72.50% 72.42% -0.08%
==========================================
Files 211 211
Lines 7386 7446 +60
Branches 766 769 +3
==========================================
+ Hits 5355 5393 +38
- Misses 1800 1820 +20
- Partials 231 233 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
StefH
changed the title
|
The delay should happen before the send. The goal here is to say, wait this long before sending this webhook. |
|
|
||
| // Call the URL | ||
| return client.SendAsync(httpRequestMessage); | ||
| var sendTask = client.SendAsync(httpRequestMessage); |
There was a problem hiding this comment.
The goal here is to wait before sending the requested amount of time. The actual mocked API request should return immediately (unless a global delay is used or a configured delay on the mapping), regardless of the webhooks. Then the webhooks should fire after the configured delay for each webhook.
There was a problem hiding this comment.
I've modified the code according to your proposal. Please see the updated PR.
There was a problem hiding this comment.
This fixes the timing and the fire and forget is working on each webhook, but the calling process remains blocked until the webhooks finish running which can be a long time if the webhook timing is big. Perhaps the UseFireAndForget should be at another level and automatically apply to all webhooks. Use the following example:
// Webhook server.Given(Request.Create().WithPath(new WildcardMatcher("/Cars", true)).UsingPost()) .WithWebhook(new Webhook { Request = new WebhookRequest { Url = "https://localhost:12345/foo1", Method = "post", BodyData = new BodyData { BodyAsString = "OK 1!", DetectedBodyType = BodyType.String }, Delay = 1000, UseFireAndForget = true } }) .WithWebhook(new Webhook { Request = new WebhookRequest { Url = "https://localhost:12345/foo2", Method = "post", BodyData = new BodyData { BodyAsString = "OK 2!", DetectedBodyType = BodyType.String }, Delay = 5000, UseFireAndForget = true } }) .RespondWith(Response.Create().WithBody("a-response"));
I'd expect here to get "a-response" more or less immediately, and then see the two webhooks trickle in separately. If you use this example, however, what you'll see is that it takes approximately ~6 seconds for the request to return. That's because even though the SendAsync isn't being awaited, the overall tasks are which include the delay. That's why in my example I had the hacky:
//await Task.WhenAll(tasks.Select(async task => await task.Invoke()));
await Task.WhenAll(tasks.Select(async task => task.Invoke()));
That await still causes the long delay in the request response. If we moved the FireAndForget so that it applies to all webhooks, we could move your FireAndForget(sendTask); out of the WebhookSender.cs and use it in the line above. What do you think?
There was a problem hiding this comment.
I think you are right. However I'm also a bit lost.
If you can comment on the PR and specify which code needs to change. I will change it.
There was a problem hiding this comment.
We want to keep webhooks individually configurable for delays, but I think the fireandforget setting needs to be a mapping setting, not a webhookmapping setting. That way we can do the await or not as a whole, not on each specific, which would allow us to wait or not in WireMockMiddleware.cs around line 226.
//await Task.WhenAll(tasks.Select(async task => await task.Invoke()));
await Task.WhenAll(tasks.Select(async task => task.Invoke()));
Let me see if I can be a little more clear.
There was a problem hiding this comment.
I've made a branch based on your branch. Of course I don't have permission to create the branch. I suppose I could fork the whole thing and do it that way, or I can try another approach to show my changes based on yours.
There was a problem hiding this comment.
Easiest would be (I think) that you fork this project, and create a new branch based on my branch (stef-webhooks) and tell me your branch, or just create a PR.
There was a problem hiding this comment.
Sent you a PR from my fork. The PR is to merge into your stef-webhooks branch since I wasn't quite sure the state of master. If you accept that merge, then you can finish your current PR with my changes to go to master (or tell me if you just want me to do a PR against master instead).
…ssic, move the new FireAndForget into the main mapping, out of individual webhook mappings making it all or nothing, update tests, change Middleware to await or not the firing of all webhooks. Update models as needed. (#804) Co-authored-by: Matt Philmon <[email protected]>
|
@mattisking And about this one: Should this be changed into: await Task.WhenAll(tasks.Select(task => task.Invoke()));?? |
|
I submitted another smaller PR. I've done this in a way that makes that little squiggle go away in Visual Studio and the associated warning but in general, the Tasks library doesn't necessarily love it when you ignore your await which is what I need to do to make this work. If I take that async away we're back to blocking on the thread. So, I'll spin it out into it's own Task. I also fixed a misunderstanding I had in adding webhooks in the example app. |
|
Sent in another PR that I updated first |
Co-authored-by: Matt Philmon <[email protected]>
|
Let me know if I can add anything. |
|
It's ok. Next Monday I will merge and create a new NuGet. |
No description provided.