Skip to content
This repository was archived by the owner on Feb 7, 2024. It is now read-only.

[2.x] Asynchronous app managers #644

Closed
wants to merge 26 commits into from
Closed

[2.x] Asynchronous app managers #644

wants to merge 26 commits into from

Conversation

mpociot
Copy link
Member

@mpociot mpociot commented Dec 17, 2020

This PR adds the ability of asynchronous application managers.
The default application manager only stores the apps in a configuration file, but if you want to store your applications in a database, the application managers need to resolve them asynchronously.

As this PR introduces quite a lot of breaking changes, due to the fact that application managers now return promises, instead of app instances, we need to refactor most of the API related tests. I would suggest that we actually start the WebSocket server in our tests and then perform actual HTTP requests on the internally started server - rather than just faking HTTP requests, as we do it right now.

The SQLite application manager still has one pending issue that needs to be resolved: clue/reactphp-sqlite#35

  • SQLite application manager
  • MySQL application manager
  • Refactor tests
  • Documentation

@mpociot mpociot requested a review from rennokki December 17, 2020 08:48
@morloderex morloderex mentioned this pull request Jan 15, 2021
14 tasks
Base automatically changed from 2.x to master January 23, 2021 15:09
@vdauchy
Copy link

vdauchy commented Apr 18, 2021

Hi @mpociot ,

I was just starting to dig into having the Apps in MySql too when I found your PR, so after looking at your code I'm super excited about it :-)

I just have one question about the AppManager::all(): array interface:

Do you think we could make it support pagination ?

I'm asking as I feel this would be the next step as when Apps are in db we could have quite a few in them :-)

Thank you and @rennokki as the code of this package is of a rare quality.

@mpociot
Copy link
Member Author

mpociot commented Apr 22, 2021

@vdauchy Do you think that'll be necessary? The all method is only needed within the Dashboard for the dropdown.
But I'd love some help on the MySQL part of this PR :)

@zek
Copy link

zek commented Apr 24, 2021

I think we should consider adding Swoole support instead ReactPHP.

@mpociot
Copy link
Member Author

mpociot commented May 3, 2021

I think we should consider adding Swoole support instead ReactPHP.

This is something that #756 is trying to implement, and way out of the scope of the current 2.x version

@DarkGhostHunter
Copy link
Contributor

The final goal of that PR is to have drivers for each async library that enables non-blocking WebSockets: Amphp, React, Ratchet, Swoole and Roadrunner (yes, WS should be landing soon on RR 2.0).

For that, major rework has to be done as it's possible to abstract as contracts each part of the WebSocket implementation, meaning, we only need to "wire" connection events and their data to their respective handler, router, and controller. In other words, abstract the client connected, the server, and the message.

It's not a small feat, but it allows the package to separate the exposed functionality from the driver itself. This could diagnose problems and bugs easier as you could just swap the driver and see if happens again.

For the case of Async Redis, use Amphp Redis unless the driver comes with its own, like Swoole does, but I haven't checked if it's mandatory for performance reasons. Since the next version could abstract the loop from the driver, calls to Redis (or even the Database) could use the loop using the default Laravel/PHP driver.

I don't know why I posted this here, but anyway...

@vdauchy
Copy link

vdauchy commented May 3, 2021

@DarkGhostHunter Thanks for sharing your thoughts here, I need to look at the mysql implementation for myself and will share what I find. I just cannot share any timelines right now as I have not much time to spare, but will as soon as I can...

@mpociot For the pagination, in a multi-tenants with multi-db this may not be needed as each db/stats table would have only one app id. However if you decide to bring all your stats to one db and one big table, then pagination might be useful if you start to have tens or hundred app ids.

@vdauchy vdauchy mentioned this pull request May 17, 2021
@mpociot mpociot closed this Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage within FPM
4 participants