soxtoby / SlackNet

A comprehensive Slack API client for .NET
MIT License
207 stars 65 forks source link

Idea of DelayedResponse #204

Closed kikaragyozov closed 3 days ago

kikaragyozov commented 2 months ago

Greetings!

I went ahead and inspected the code, and it seems to be that if DelayedResponse is set to false, you will queue the completion delegates to run after the response has completed, via HttpResponse.OnCompleted.

But it seems that inside those delegates, we still wait for the completion of user's handler code.

What's the idea here for awaiting it? Is it so that things like IHttpContextAccessor does not get disposed, and/or something else along those lines?

I was just fascinated by the code and started theory crafting what would happen if those delegates never completed, as well as wanting to learn the motivations behind await-ing the user code. I'm guessing if you have a very long running job, even though you're returning responses immediately to slack, the server would be under stress, and will be forced to spawn new threads, am I right in assuming so? Or, even though ASP.NET Core is await-ing the delegates, no new threads will be required to be made (I believe a request in ASP.NET Core is not fully treated as completed, unless the delegates also complete, but then again I'm not sure if that would result in some kind of thread exhaustion)

Thanks!

soxtoby commented 2 months ago

Hi @kikaragyozov. Using HttpResponse.OnCompleted was the easiest way I could find to run code after a response has been returned to Slack. It takes in a Func<Task>, and it seems implied to me that the returned task should complete when the async operation is complete, which is why user tasks are awaited. The alternative would be to return Task.CompletedTask, misrepresenting the state of the operation, which would almost certainly lead to unexpected behaviour.

As to why HttpResponse.OnComplete takes in a Func<Task>, looking at the Kestrel source, it does indeed look like the request context is disposed after the tasks complete. Since the switch from pre-response to post-response can happen in the middle of a handler function (when the Responder is invoked), losing the request context would be surprising I think.

I haven't investigated exactly what happens if handlers never return, but it's probably not good. I don't think it'd be much different to never-ending tasks returned from normal ASP.NET request handlers though.

FWIW, the async handler behaviour in SlackNet isn't meant for long-running tasks. Slack often expects a response within about 3 seconds (including roundtrip time), and it's easy to go over that limit just by making a couple of API requests. Being able to respond early helps keep you in that 3 second window, while allowing you to react a couple seconds later. As per Microsoft's advice, long-running running tasks should be completed outside of HTTP requests.

Hope this helps.

stale[bot] commented 3 days ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

kikaragyozov commented 3 days ago

Hi @kikaragyozov. Using HttpResponse.OnCompleted was the easiest way I could find to run code after a response has been returned to Slack. It takes in a Func<Task>, and it seems implied to me that the returned task should complete when the async operation is complete, which is why user tasks are awaited. The alternative would be to return Task.CompletedTask, misrepresenting the state of the operation, which would almost certainly lead to unexpected behaviour.

As to why HttpResponse.OnComplete takes in a Func<Task>, looking at the Kestrel source, it does indeed look like the request context is disposed after the tasks complete. Since the switch from pre-response to post-response can happen in the middle of a handler function (when the Responder is invoked), losing the request context would be surprising I think.

I haven't investigated exactly what happens if handlers never return, but it's probably not good. I don't think it'd be much different to never-ending tasks returned from normal ASP.NET request handlers though.

FWIW, the async handler behaviour in SlackNet isn't meant for long-running tasks. Slack often expects a response within about 3 seconds (including roundtrip time), and it's easy to go over that limit just by making a couple of API requests. Being able to respond early helps keep you in that 3 second window, while allowing you to react a couple seconds later. As per Microsoft's advice, long-running running tasks should be completed outside of HTTP requests.

Hope this helps.

This most definitely helped, thank you!