elsa-core
elsa-core copied to clipboard
elsa-workflows
Update WriteHttpResponse.cs to call response.CompleteAsync()
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.
Hello Thanks for this, it seems to fix some part of the issue. Have you do some test on Error handling in the Workflow?
If the workflow throw an exception after the HttpResponse, the code seems to try to WriteAsync with the HttpContext.Response which I think will throw another exception.
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. In that light, I also wonder if there may exist a scenario where one might want to actually wait for the full workflow to complete before flushing the response? If yes, perhaps this behavior should be configurable through a checkbox on the activity. What do you think @ciaranodonnell @jdevillard ?
Good point. Perhaps that error handling code needs to check to see if a response is already being sent, and if so, ignore it. In that light, I also wonder if there may exist a scenario where one might want to actually wait for the full workflow to complete before flushing the response? If yes, perhaps this behavior should be configurable through a checkbox on the activity. What do you think @ciaranodonnell @jdevillard ?
What will be the difference between placing the HttpResponse activity in the middle with a checkbox to wait until the end and placing the activity at the end of the Workflow?
And also if the Workflow is Suspended the Middleware release the client and a response is sent, so even with the checkbox, we could have some behavior not easily visible by the user.
Last though, if we really wait to respond to the caller to handle something wrong, the caller will be able to handle some internal exception about the infrastructure. For example, today, there is a delay between the last activity (httpResponse) and the end of the workflow because the middleware flush the call after all the persistence behavior. If there is any issue on the persistence, the call will have a 400/500 , the idea is to determine what is the concern of the caller about the internal Workflow mecanism or just the execution of the workflow.
What will be the difference between placing the HttpResponse activity in the middle with a checkbox to wait until the end and placing the activity at the end of the Workflow?
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?
@ciaranodonnell what scenario do you have in mind that requires the HTTP Response activity to happen before the end of the workflow?
@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.
Perhaps the scenario is to continue the remainder of the workflow asynchronously from a background process?
yes I think it could be good, can we consider this implementation as a second step? and for now just :
- call
CompleteAsync() - add the test to avoid issue if
CompleteAsync()has already been called
This will solve other Perf issue in my sense.
@jdevillard Yes, we could just add the call to CompleteAsync, but then there's the potential for an error happening in subsequent parts of the HTTP request context, which the client will now not receive, as per your earlier observation. This may or may not be acceptable, depending on a case by case and depending on who you ask. For this reason, I propose to bind it to a checkbox, so that the user can make a conscious choice whether or not to accept this risk. In case of performance requirements, such as the one you described, this could be an acceptable risk, so you tick the checkbox.
What will be the difference between placing the HttpResponse activity in the middle with a checkbox to wait until the end and placing the activity at the end of the Workflow?
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?
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. As a user I expect that when the "Respond to the call" activity completes and moves on that the response is sent. If I want the response to be contingent on remaining activities, I would put it at the end.
@jdevillard Yes, we could just add the call to
CompleteAsync, but then there's the potential for an error happening in subsequent parts of the HTTP request context, which the client will now not receive, as per your earlier observation. This may or may not be acceptable, depending on a case by case and depending on who you ask. For this reason, I propose to bind it to a checkbox, so that the user can make a conscious choice whether or not to accept this risk. In case of performance requirements, such as the one you described, this could be an acceptable risk, so you tick the checkbox.
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?
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. Your code was merged in #5446