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

Laracon 2024 #52710

Merged
merged 49 commits into from
Sep 11, 2024
Merged

Laracon 2024 #52710

merged 49 commits into from
Sep 11, 2024

Conversation

taylorotwell
Copy link
Member

No description provided.

@taylorotwell taylorotwell marked this pull request as draft September 9, 2024 14:18
src/Illuminate/Concurrency/composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
* @param array{ seconds?: int, owner?: string }|null $lock
* @return TCacheValue
*/
public function flexible($key, $ttl, $callback, $lock = null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious what you think about calling this (or aliasing it to) Cache::throttle() or Cache::debounce(), since it is literally throttling/debouncing the refreshing of the cached value. Regardless can't wait to use it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another is why a 2-element array for ttl and not spreading it as, say, ($key, $ttlFresh, $ttlStale, ...)

* Concurrency Improvements

1. Add a working tests for Concurrency
2. Update composer.json to include replace `illuminate/concurrency`
3. Update PHP versions

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* Apply fixes from StyleCI

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

* wip

Signed-off-by: Mior Muhammad Zaki <[email protected]>

---------

Signed-off-by: Mior Muhammad Zaki <[email protected]>
Co-authored-by: StyleCI Bot <[email protected]>
Comment on lines +96 to +100
return (new ServeFile(
$disk,
$config,
$this->app->isProduction()
))($request, $path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to resolve this via the container? That would allow for easier modification.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you could use Storage::serveUsing(fn)

@heychazza
Copy link

Hey @taylorotwell, thanks for such a fantastic Laracon! The Concurrency stuff has been a huge feature I'm most excited for, so I gave it a go using your PR to tinker.

I'm not sure if intentional, but the php binary at https://github.com/laravel/framework/blob/laracon-2024/src/Illuminate/Concurrency/ProcessDriver.php#L31 is never found when using Laravel Herd.

I presume it's intended that we only use sync during local dev, and process for production? I understand this is still a draft, but if release is tomorrow I wanted to point out.

Thanks a ton for this regardless :)

@donnysim
Copy link
Contributor

@taylorotwell the PHP binary should definitely be configurable, we have a development server where you switch PHP per project basis and the global PHP is not the one that is used for the project and accessed via php8.2 artisan etc.. Maybe introduce a config and env variable?

@osbre
Copy link
Contributor

osbre commented Sep 10, 2024

I wonder if Octane would have its own concurrency driver since Swoole/OpenSwoole have a feature called "task workers".

I also heard that the number of threads an OS could have/handle is limited. Any possible cases of requests spawning too many threads causing performance issues?

@taylorotwell taylorotwell marked this pull request as ready for review September 11, 2024 20:05
@taylorotwell taylorotwell merged commit c44aa73 into 11.x Sep 11, 2024
32 checks passed
@taylorotwell taylorotwell deleted the laracon-2024 branch September 11, 2024 20:06
@devajmeireles
Copy link
Contributor

@taylorotwell

CleanShot 2024-09-11 at 18 55 45

Also, you forgot to add the documentation blocks for the Competition facade.

protected function forgetDuplicates(): self
{
$this->callbacks = collect($this->callbacks)
->reverse()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taylorotwell not sure, but I think we don't need the double reverse(), right?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@decadence
Copy link
Contributor

I'm trying to call Concurrency::run but got:

Illuminate\Contracts\Container\BindingResolutionException
Unresolvable dependency resolving [Parameter #0 [ <required> $app ]] in class Illuminate\Support\MultipleInstanceManager

Also it seems like defer uses sync driver.

How to configure this facade?

@Jubeki
Copy link
Contributor

Jubeki commented Sep 12, 2024

Please open an issue if you have found a problem. Comments in already merged PRs are mostly not looked after. You can also send in a PR to fix the problem directly.

@decadence
Copy link
Contributor

I'm not sure if this is a issue. Maybe I'm just doing something wrong.

@mahmoudmohamedramadan
Copy link
Contributor

What is the difference between the defer method and dispatching the job after response?

I see it's just a shorthand furthermore, I see that the job class is more proper for doing simple or complex actions!

@timacdonald
Copy link
Member

timacdonald commented Sep 29, 2024

defer has a different feature set than dispatching jobs after the response, such as cancelling on bad status codes or always running.

defer is not tied to the queuing system. This means that models are not serialized / re-queried, reducing potential database queries, and when captured models are deleted within the request the closure will still run, unlike dispatching jobs are the response.

If the tradeoffs of using dispatch after response work in your favour, you should keep using it.

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.