elsa-workflows / elsa-core

A .NET workflows library
https://v3.elsaworkflows.io/
MIT License
6.48k stars 1.2k forks source link

Flushing and closing connection with HTTP Response acitivity #1144

Open tomy2105 opened 3 years ago

tomy2105 commented 3 years ago

I have a feeling that http connection is closed/flushed only when workflow instance is finished or suspended. I have workflow which has HTTP Response activity near beginning then some "lengthier" processing and then it finisheds.

And in browser I see HTTP Response text immediately but the page keeps "loading" till the workflow instance is finished, only then it stops.

This probably happens because of chunked content encoding or something about that.....

Are HTTP Response blocks envisioned so there could be two? So both responses are "concatenated"? If yes then we need to be able to mark last one so it gets "flushed". If not (the way I'd want it to be) then content should be "flushed" immediately with first one.

sfmskywalker commented 3 years ago

Are HTTP Response blocks envisioned so there could be two? So both responses are "concatenated"?

They are not, but I have been thinking about a way to allow for having more than one HTTP Response activity to concatenate the response. When you try to do this right now, you should be getting an error because of response headers already being sent down to the client. But there's probably a way to keep on sending content to the client without also sending headers.

I presume that when you want to flush the content, you want the page to stop loading. Which will probably happen when we close the connection from the server, without stopping the workflow from continuing (since it's not using the RequestAborted cancellation token anymore).

Do you want to submit a PR with an HTTP Response activity that can be used multiple times, and maybe with a checkbox (that is checked by default) called "Flush"?

tomy2105 commented 3 years ago

Frankly, I don't see a point :) in being able to invoke HTTP Response twice. If you want to concatenate several things into your response you can do it using workflow variables and invoke only one at the end (or whenever you are done).

And this seems to be along the lines the code is currently written:

        if (response.HasStarted)
            return Fault(T["Response has already started"]!);

Just flush it/close connection after first response has been written if that is possible (haven't checked code in detail).

sfmskywalker commented 3 years ago

I don't think it's a good idea to just close the response stream from within the HttpEndpointMiddleware. Even if it were, it's probably not a good idea or at the very least an incorrect "fix" for what you're trying to achieve.

Which is, as I understand it, a way for the workflow to continue running after the HTTP Response activity is done. Workflows with this activity as a trigger will execute the full workflow, so only after that completes its burst of execution will the middleware complete and continue on to the next one, if any.

My understanding is that IIS Express buffers the response anyway (probably depending on some configuration).

Regardless of the above being correct or not, perhaps a better idea is simply for the workflow to continue running asynchronously in the background so that further execution doesn't block the middleware from completing.

In that case, we might add a setting to WriteHttpResponse so that it returns some sort of DispatchResult to tell the workflow that the current burst of execution is done and have it resume the workflow from a queue.

Alternatively, we might introduce a new activity that is like the Timer activity, causing the workflow to get suspendend ans then resumed.

In which case, you might just use the Timer activity right away with a delay of 0 seconds, achieving exactly what you want.

Thoughts?

tomy2105 commented 3 years ago

I would agree with you that closing is not the best way to approach this (due to number of reasons you described).

I don't like Timer workaround because it would mean having Timer after every WriteResponse which doesn't look nice.

Would definitely vote for the option of WriteHTTPResponse being able to signal finishing of current burst (which would at then end flush the response) and the rest of the workflow executed asynchronously.

zman2000 commented 3 years ago

In which case, you might just use the Timer activity right away with a delay of 0 seconds, achieving exactly what you want.

I had a same issue with the HTTP response. Tried the workaround with the Timer and delay must be > 0, otherwise HTTP response will still be delayed. I used Elsa 2.0 GA.

sfmskywalker commented 3 years ago

@zman2000 You’re right, I forgot that the timer activities have a small optimization where it skips scheduling altogether if the configured delay is zero or less.

tomy2105 commented 3 years ago

Considering the "restrictions" (at least when using Hangfire) of probably delay not working "correctly" if under 1 second, I'd definitely want to see closing/flushing response as a checkbox inside HTTP Response Activitiy :).

terry-delph commented 2 years ago

I was looking at this issue as I wanted to understand the mechanics of the HttpEndpointMiddleware and the WriteHttpResponse.

It is not possible to end the Http Request from the workflow itself, as there is no such thing as Request.End() in .Net Core. All requests must go through the middleware pipeline, and traverse back through them all until it can eventually return the request as being completed. Based on this, the only way to complete the request and allow the workflow to continue is by suspending the workflow. The HttpEndpointMiddleware will wait until the workflow suspends, the remaining middleware will be processed and the request will end.

If a checkbox was added to the WriteHttpResponse it could be implemented as follows:

/// <summary>
/// A value indicating whether the activity should suspend itself after writing the response.
/// This allows the request to end as quickly as possible if other activities require to be executed after the request ends.
/// </summary>
[ActivityInput(
Hint = "A value indicating whether the activity should suspend itself after writing the response. This allows the request to end as quickly as possible if other activities require to be executed after the request ends.",
SupportedSyntaxes = new[] { SyntaxNames.Literal, SyntaxNames.JavaScript, SyntaxNames.Liquid })]
public bool SuspendAfterWrite { get; set; }
... existing code
await WriteContentAsync(context.CancellationToken);

if(SuspendAfterWrite)
{
     return Combine(Suspend(), new ScheduleWorkflowResult(new Instant()));
}

return Done();

I gave this a try and result was as I expected.

@sfmskywalker Is this a suitable approach to solve this problem? Would you expect a more advanced solution that should be handled in the middleware itself? Would this approach introduce issues elsewhere?