swoft-cloud / swoft

🚀 PHP Microservice Full Coroutine Framework
https://swoft.org
Apache License 2.0
5.58k stars 786 forks source link

WsMessageDispatcher - Response without FD #890

Closed goddanao closed 5 years ago

goddanao commented 5 years ago

The Response object isn't initialized with FD. Seems a bug in 2.0.5-beta

class WsMessageDispatcher // extends \Swoft\Concern\AbstractDispatcher
{
    public function dispatch(Server $server, Request $request, Response $response): void
    {
        $fd = $request->getFd();
        // Need this to make it work as before 2.0.5-beta
        $response->setFd($fd);
inhere commented 5 years ago

😯 By default, it is not set to receive users.

you can:

$response->to($fd)->setContent('hi')->send();

Do you want this to be the default?

goddanao commented 5 years ago

The issue i found in my code is that i can see you changed the way WsDispatcher handle Messages as response from WsControllers. (https://github.com/swoft-cloud/swoft-websocket-server/commit/94f47a6ac0b5e12d3fcff63d8baed55055a22c25#diff-33fae66f8afcb5aeacc1a79a07072ef1L107).

Prior 2.0.5-beta returning a Message from a WsController (as i do) will make WsDispatcher build a Response using Request->fd.

Now if i return anything but Response from the WsController it will be not default dispatched to the requesting client, unless as you suggested, you build a full Response with the Request->fd.

My 2c: I found the previous implementation more useful, since 90% of times i'd like to return data to the requester, so i don't need to build a Response in the WsController. And when it's really needed i stiill can Return a full flagged Response with target fd(s). With the current implementation, the WsController method signature can be changed with something like ": ?Response", since returning anything else don't make the response to be delivered to the requester.

Let me know if i misunderstood or miss something.

inhere commented 5 years ago

hi @goddanao thanks you suggestion.

I will use Request->fd as the default receiver.

There are other suggestions?

goddanao commented 5 years ago

hi @inhere, glad to contribute as i can.

More questions (maybe some suggestions) will come ;-) , since i'm currently trying to port my own app framework (php/ratchet/redis/mysql/zmq/extjs) to swoft, and i'm playing with:

I'd like to not reinvent the wheel, so i'm asking, are you going to release something like queue / job queue support (maybe with future releases of swoft-kafka?)

Thankyou

inhere commented 5 years ago

😄 If you are willing and have time, you are very welcome to implement a swoft-kafka.

inhere commented 5 years ago

refer swoft-cloud/swoft/issues/862

goddanao commented 5 years ago

hi @inhere, thank you.

Consider implementing more entry points and maybe Annotations like @WsExtension, @WsSubProtocol. This opens the ws server to interesting scenarios, like permessage-deflate or eg. wamp subprotocol implementation ( look at https://github.com/ratchetphp/Ratchet/tree/master/src/Ratchet/Wamp, i think it could be adapetd to swoft with some effort ).

About Swoft-Kafka, i think good a starting point to be inspired from could be https://github.com/weiboad/kafka-php, it's based on amp loop, could be adapted to swoft quite easily. I'll investigate..

inhere commented 5 years ago

hi @goddanao in message action method. How do you usually return data or respond to a message?

goddanao commented 5 years ago

hi @inhere, in action methods i usually return:

inhere commented 5 years ago

ok, thanks you reply @goddanao