-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update WriteHttpResponse.cs to call response.CompleteAsync() #5268
Conversation
Hello Thanks for this, it seems to fix some part of the issue. If the workflow throw an exception after the HttpResponse, the code seems to try to Need to check some tests. |
Good point. Perhaps that error handling code needs to check to see if a response is already being sent, and if so, ignore it. |
What will be the difference between placing the And also if the Workflow is Last though, if we really |
That's a great point; after all, why would the author of the workflow put the HTTP Response activity before the remaining activities. @ciaranodonnell what scenario do you have in mind that requires the HTTP Response activity to happen before the end of the workflow? The concern with sending back the response while the workflow is still executing, is what happens in case of errors later on, like @jdevillard explains. Perhaps the scenario is to continue the remainder of the workflow asynchronously from a background process? |
@sfmskywalker on this point, I've the same use case and it is the following. I've got a Stream Log Processing of a SaaS application which flush his data calling an Workflow Endpoint of Elsa. But this call need to execute in less than 5 sec, otherwise, the client close the connexion and throw an error. So I've to acknowledge quickly that I've considered the request and be able to handle this (maybe later) but the Caller has done his work.
yes I think it could be good, can we consider this implementation as a second step? and for now just :
This will solve other Perf issue in my sense. |
@jdevillard Yes, we could just add the call to |
We call a Workflow with Http and have the respond activity be the very next activity that confirms the workflow has started. After that all out progress in the workflow is published as events. |
I don't think the checkbox is unreasonable as a Back-Compat feature considering this is already out in the wild. I can add the check box today and add a test to that behavior. I'll try to update the PR today. |
@ciaranodonnell On second thought, I think the expectation that you described makes sense. So maybe, instead of doing the checkbox thing, it should be a system-wide flag that controls the behavior. Let's consider the current behavior as faulty. To opt-in into the corrected behavior (the one you're proposing via this PR), the developer can set this flag. This way, we can change the flag in a later version, where one would have to opt-out if they want to stick to the current behavior. @jdevillard What do you think? |
Ok for me ! |
Hello @ciaranodonnell , Hope your are doing well. Do you agree with the flag concept ? Will you get some time to update your PR ? |
@ciaranodonnell Thanks for hitting this issue and your contribution. |
I think this is the fix to bug #5267
If we call the CompleteAsync() on the response it will flush the response to the client. This fixes the issue in our testing and the response is written immediately.
This change is