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 centrifuge worker prototype #123

Closed
wants to merge 1 commit into from

Conversation

FluffyDiscord
Copy link
Contributor

This PR aims to implement support for Centrifuge worker https://roadrunner.dev/docs/plugins-centrifuge using symfony event dispatcher.

As it is, it works, but it should probably receive it's own middleware stack, mainly for doctrine and sentry. I am not sure about it's own kernel_reboots, I guess its also needed?

The centrifugo middleware will need to be separated from http middlewares, probably simply using different namespaces, so we do not touch http middlewares and keep the yaml config. Middlewares would then sort by the interface they implement.

If we do want to add kernel reboots, then we will need to change the yaml config structure, which could be breaking change if we want to make them universal

@FluffyDiscord FluffyDiscord changed the title DRAFT: Add centrifuge worker prototype Add centrifuge worker prototype Jul 21, 2023
@Baldinof
Copy link
Owner

Thank you!

Sorry for the delay, I'll try to have a look this week.

@FluffyDiscord
Copy link
Contributor Author

Please let me know which things need to be added or adjusted so this PR aligns with this bundle's intentions

@FluffyDiscord
Copy link
Contributor Author

don't forget also this PR this week so we can have offical support! :]

Copy link
Owner

@Baldinof Baldinof left a comment

Choose a reason for hiding this comment

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

Thank you!

Honestly I never used Centrifuge :D

Comment on lines +41 to +52
$eventClass = match (true) {
$request instanceof Request\Connect => ConnectEvent::class,
$request instanceof Request\Publish => PublishEvent::class,
$request instanceof Request\Refresh => RefreshEvent::class,
$request instanceof Request\SubRefresh => SubRefreshEvent::class,
$request instanceof Request\Subscribe => SubscribeEvent::class,
$request instanceof Request\RPC => RPCEvent::class,
$request instanceof Request\Invalid => InvalidEvent::class,
default => throw new \RuntimeException(sprintf("Unsupported \$request type '%s'", $request::class)),
};

$event = new $eventClass($request);
Copy link
Owner

Choose a reason for hiding this comment

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

I would prefer it to be like this:

Suggested change
$eventClass = match (true) {
$request instanceof Request\Connect => ConnectEvent::class,
$request instanceof Request\Publish => PublishEvent::class,
$request instanceof Request\Refresh => RefreshEvent::class,
$request instanceof Request\SubRefresh => SubRefreshEvent::class,
$request instanceof Request\Subscribe => SubscribeEvent::class,
$request instanceof Request\RPC => RPCEvent::class,
$request instanceof Request\Invalid => InvalidEvent::class,
default => throw new \RuntimeException(sprintf("Unsupported \$request type '%s'", $request::class)),
};
$event = new $eventClass($request);
$event = match (true) {
$request instanceof Request\Connect => new ConnectEvent($request),
$request instanceof Request\Publish => new PublishEvent($request),
$request instanceof Request\Refresh => new RefreshEvent($request),
$request instanceof Request\SubRefresh => new SubRefreshEvent($request),
$request instanceof Request\Subscribe => new SubscribeEvent($request),
$request instanceof Request\RPC => new RPCEvent($request),
$request instanceof Request\Invalid => new InvalidEvent($request),
default => throw new \RuntimeException(sprintf("Unsupported \$request type '%s'", $request::class)),
};

$request instanceof Request\Subscribe => SubscribeEvent::class,
$request instanceof Request\RPC => RPCEvent::class,
$request instanceof Request\Invalid => InvalidEvent::class,
default => throw new \RuntimeException(sprintf("Unsupported \$request type '%s'", $request::class)),
Copy link
Owner

Choose a reason for hiding this comment

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

Can we have a dedicated exception?

\assert($processedEvent instanceof CentrifugeEventInterface);

$response = $processedEvent->getResponse() ?? match ($eventClass) {
ConnectEvent::class => new ConnectResponse(),
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe instead we can set a default response instance in the events? So $processedEvent->getResponse() would never return null? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't think about it, but it makes sense.

$request->respond($response);
}
} catch (\Throwable $throwable) {
$request->disconnect(500, 'Server error');
Copy link
Owner

Choose a reason for hiding this comment

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

Should we do more stuff here?

In the http context, exception are converted into http responses (unles something happen of the Symfony request handling), so we almost never reach this catch in the http worker.

Maybe we should re-use the kernel_reboot.allowed_exceptions list and do not disconnect if it's whitelisted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the context of websocket I think we should always disconnect on any exception by default. You are expected to send valid payload that the other side can parse without errors, since there is no "default" payload that you can send that will be always processed. It's up to the user to handle his own errors as they can always change the response event to something else. Centrifuge has also auto reconnect feature that handles disconnects for you.

@@ -30,7 +30,8 @@
"spiral/roadrunner-worker": "^3.0.0",
"spiral/goridge": "^4.0",
"psr/log": "^1.1 || ^2.0 || ^3.0",
"spiral/roadrunner-http": "^3.0"
"spiral/roadrunner-http": "^3.0",
"roadrunner-php/centrifugo": "^2.0"
Copy link
Owner

Choose a reason for hiding this comment

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

This should be in the suggest section

@FluffyDiscord
Copy link
Contributor Author

Do you have any suggestions about the middleware structure in the context of websockets? There should definitely be a Doctrine and Sentry middleware - Doctrine to reset the EntityManager and Sentry to send all caught events, just like in the context of HTTP.

I am not sure which would be better, if to add event listener (which could be cancelled by user by mistake) or equivalent of the HTTP stack, but modified to run before and after instead of the current Generator approach (since we are dispatching the websocket message as event)

@michaelgracious
Copy link

Is this PR is stalled? This is something i'd love to see in the bundle so we can use Centrifuge

@FluffyDiscord
Copy link
Contributor Author

Is this PR is stalled? This is something i'd love to see in the bundle so we can use Centrifuge

yes, I won't be adding anything here, I have made my own bundle with centrifuge

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.

3 participants