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

Fix max_execution_time config doesn't work #510

Merged
merged 4 commits into from
Apr 20, 2022

Conversation

sy-records
Copy link
Contributor

@sy-records sy-records commented Apr 18, 2022

Fix #470

Re-create the new Swoole response object to use it to send the response.

image

@sy-records sy-records force-pushed the fix/max_execution_time branch from 7ab27fe to 5a8aef4 Compare April 18, 2022 11:20
@taylorotwell
Copy link
Member

What is fd? Can you explain more what this PR is actually doing?

@sy-records
Copy link
Contributor Author

The client connection $fd number

you can see https://openswoole.com/docs/modules/swoole-http-response-create

@taylorotwell
Copy link
Member

This is available on all versions of Swoole? Both Swoole and OpenSwoole?

@sy-records
Copy link
Contributor Author

swoole is supported since v4.6.0+, openswoole is forked from v4.7.0, so both are supported.

But openswoole has a max_request_execution_time configuration, so I didn't add it for it.

@smortexa
Copy link
Contributor

@sy-records I think OpenSwoole should be added because max_request_execution_time config doesn't work on Octane yet (openswoole/ext-openswoole#136).

@sy-records
Copy link
Contributor Author

Got it, thanks for the feedback! @smortexa

@taylorotwell
Copy link
Member

Sorry - still trying to understand this. How does this fix the problem with a worker hanging? Isn't this response being created on the master process?

@sy-records
Copy link
Contributor Author

Since the worker process is forced to kill, it is understood to have been detached at this point.
But to keep the response, we use Response::create to recreate the response

@taylorotwell taylorotwell merged commit f03913a into laravel:1.x Apr 20, 2022
@sy-records sy-records deleted the fix/max_execution_time branch April 20, 2022 17:36
@spire-mike
Copy link

@sy-records @driesvints it appears that this timeout doesn't work nicely with atomic locks. For example, with config.octane.max_execution_time = 30, and hitting the following route:

Route::get('/test-route', function () {
  Cache::lock('test-lock')->get(function () {
    sleep(60);
  });

  return response('');
});

The test-lock gets set, but it never gets released when the 408 response is sent. Is there some way to 'cleanup' after the request times out?

I'm on Laravel 8.83.11, Octane 1.2.8 using swoole.

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

Successfully merging this pull request may close these issues.

max_execution_time config doesn't work
6 participants