Skip to content
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

Add function to finalize HTTPServerResponse stream manually #424

Closed
ilya-stromberg opened this issue Dec 4, 2013 · 12 comments
Closed

Add function to finalize HTTPServerResponse stream manually #424

ilya-stromberg opened this issue Dec 4, 2013 · 12 comments

Comments

@ilya-stromberg
Copy link
Contributor

This function should finalize HTTPServerResponse.bodyWriter and free all HTTPServerResponse resources like (keep-alive) connection. It can help to avoid unnecessary client waiting if we need to run some tasks after response to the client (like save information into DB, save information into file, send an e-mail, save information to the log and etc.).
It allows to process the same number of connections as without this feature, but client will receive response faster.
Note that FastCGI supports this option and php-fpm implements it.
See also: http://forum.rejectedsoftware.com/groups/rejectedsoftware.vibed/thread/8086/

@s-ludwig
Copy link
Member

s-ludwig commented Dec 4, 2013

Why not use runTask for any after-response processing?

@s-ludwig
Copy link
Member

s-ludwig commented Dec 4, 2013

There will always be some resources that are only freed when the request callback returns (a lot would have to be changed otherwise), but runTask will handle all this naturally.

@ilya-stromberg
Copy link
Contributor Author

Why not use runTask for any after-response processing?

Will request callback wait until runTask not finished it job? If no, it can be also good alternative.

@s-ludwig
Copy link
Member

s-ludwig commented Dec 4, 2013

No, the request callback will continue and return normally after the new task has been issued, causing the connection to be free for new requests (or closed). The new task will continue to run in the background. The only gotcha is that the task is not allowed to access the request/response objects, or any data contained in them, because at least with -version=VibeManualMemoryManagement, those may have been freed already. Unfortunately, D's scope is not (yet, hopefully) powerful enough to enforce that.

@ilya-stromberg
Copy link
Contributor Author

OK, I see. Yes, in that case runTask really good solution (except, probably, spend a little additional resources for Task scheduling).
I still think that this feature can be useful, but agree that it's only minor improvement. So, Sönke, you should decide if you want to have it in future and keep or close this issue.

@s-ludwig
Copy link
Member

s-ludwig commented Dec 4, 2013

except, probably, spend a little additional resources for Task scheduling

Yes, but that's really minor, except if TaskLocal storage is used heavily. It's mostly just the fiber switch, which is pretty fast.

I'll leave it open because there are some things that are not yet optimal (e.g. there is res.bodyWriter.finalize(), but it only partially finalizes the request). But I need to think that through some more in a calm moment.

@ilya-stromberg
Copy link
Contributor Author

I'll leave it open because there are some things that are not yet optimal (e.g. there is res.bodyWriter.finalize(), but it only partially finalizes the request). But I need to think that through some more in a calm moment.

OK, thanks.

Why not use runTask for any after-response processing?

I think that Task can't be used only if I want run after-response processing as soon as possible, not after 1_000 another tasks (and, maybe, I'll use blocking API for it). So, this addition can be useful, but I agree that it can help only in a few cases.

@s-ludwig
Copy link
Member

s-ludwig commented Dec 4, 2013

I think that Task can't be used only if I want run after-response processing as soon as possible, not after 1_000 another tasks (and, maybe, I'll use blocking API for it). So, this addition can be useful, but I agree that it can help only in a few cases.

Sorry, I didn't get what you mean with 1000 tasks. Can you try to explain with different words? I general I don't see a problem with the task approach.

But if (non-vibe.d based) blocking APIs are used, the work needs to happen in a different thread, response finalization doesn't help there either.

@ilya-stromberg
Copy link
Contributor Author

Sorry, I didn't get what you mean with 1000 tasks. Can you try to explain with different words? I general I don't see a problem with the task approach.

If I understand Vibe.d fiber model correctly, Vibe.d have own scheduling for Tasks. So, when Vibe.d process current Task, another 1000 Tasks can be added (for example, it can be new client requests). Therefore, if I use runTask to create new after-response processing, it will be run after existing 1000 Tasks, but not right now. So, if I want to start after-response processing as soon as possible I can't use runTask because I haven't got any guaranties about actual time of job starting.
In this case I can start after-response processing in current fiber to make sure that job already started. I can use Vibe.d non-blocking API if I don't worry about finish of job (for example, send non-blocking insert request to the DB). I can use non-Vibe.d blocking API if I want to make sure that job already done (of course, only if this job can be done fast).

Or, maybe, we have priority tasks that runs before any other tasks? It can solve (almost) all our problems.

@s-ludwig
Copy link
Member

s-ludwig commented Dec 4, 2013

Oh I see, no all of those 1000 tasks will be scheduled implicitly by the OS kernel based on the order of finished I/O operations. If the CPU load is below 100% (i.e. the application is I/O bound or partially idle), the new task should be processed more or less immediately and it's overall run time depends on how it competes for I/O resources with the other tasks. Also, the task will start to execute immediately during runTask and run until the first yield or I/O operation is reached before runTask returns, so it has full precedence for the first execution round.

@ilya-stromberg
Copy link
Contributor Author

OK, thanks for explanation.

@s-ludwig
Copy link
Member

HTTPServerRespose.finalize() was added in 77284b9. See also #1347.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants