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

Separate Factory for Server Instance #40

Merged
merged 25 commits into from Nov 13, 2018

Conversation

Projects
None yet
3 participants
@gsteel
Contributor

gsteel commented Nov 3, 2018

  • Are you creating a new feature?
    • Why is the new feature needed? What purpose does it serve?

At the moment, if you want to setup, say, an onTask event handler in the HTTP server, in order to get a server instance, you have to call ServerFactory::createSwooleServer() in a delegator or something, the problem with that is that you circumvent the options in the $appendOptions of that method when they are called from the request handler runner context. This would render CLI options useless. By creating a factory responsible solely for the instantiation of the server process, it allows further configuration by way of delegator factories, or by replacing the shipped factory completely.

Furthermore, it's not like you can peg a delegator factory onto the ServerFactory currently because you might end up in a situation where you want to add a callback to the server, but it's already running…

It might be a better strategy to drop ServerFactory completely (Or repurpose it as defined in the pull) and then inject the server instance into the request handler runner, ensuring that it is not already running (As that's the runner's job)

If you'll consider a pull of this nature, I'll put some work into the tests and docs.

Cheers

  • How will users use the new feature?
  • Base your feature on the develop branch, and submit against that branch.
  • Add only one feature per pull request; split multiple features over multiple pull requests
  • Add tests for the new feature.
  • Add documentation for the new feature.
  • Add a CHANGELOG.md entry for the new feature.
@weierophinney

This comment has been minimized.

Member

weierophinney commented Nov 5, 2018

Thanks for starting this - I was planning on doing something similar, as I read about the task worker features after we did the stable release!

Take a look at the build failures, and please write tests for the new functionality. Once you have, I'll review!

Show resolved Hide resolved test/ServerFactoryTest.php Outdated
use const SWOOLE_SOCK_UDP6;
use const SWOOLE_UNIX_DGRAM;
use const SWOOLE_UNIX_STREAM;
class ServerFactory

This comment has been minimized.

@gsteel

gsteel Nov 6, 2018

Contributor

Would it be better to remove ServerFactory completely? The request runner could then just accept a swoole server and ->set() whatever options it needs to before finally starting the swoole server

This comment has been minimized.

@weierophinney

weierophinney Nov 6, 2018

Member

Doing so would be a BC break, for a few reasons:

  • Anybody extending the class now is no longer having their class called, which will lead to bugs.
  • The constructor of the SwooleRequestHandlerRunner currently accepts the ServerFactory, which means removing it would require a signature change in the constructor of that class.

As it is, however, the ServerFactory now has a changed constructor, which is already a BC break.

As such, I'd argue we go full tilt, and make all the changes. We can release a 2.0 almost immediately, and let users know they can adopt it immediately if they were not extending any of these factories.

@weierophinney

This looks good, and your comment on the ServerFactory makes sense (see more below).

As such, here's the rest of what needs to be done:

  • Rebase against the develop branch, and change the PR target to the develop branch.
  • Remove ServerFactory, and move the logic for injecting options into SwooleRequestHandlerRunner::startServer().
  • Update the constructor of SwooleRequestHandlerRunner to typehint against the Swoole\Http\Server instead of the ServerFactory.
  • Update any relevant existing documentation
  • Write some documentation demonstrating a delegator factory that registers tasks!

Thanks for the great work so far!

use const SWOOLE_SOCK_UDP6;
use const SWOOLE_UNIX_DGRAM;
use const SWOOLE_UNIX_STREAM;
class ServerFactory

This comment has been minimized.

@weierophinney

weierophinney Nov 6, 2018

Member

Doing so would be a BC break, for a few reasons:

  • Anybody extending the class now is no longer having their class called, which will lead to bugs.
  • The constructor of the SwooleRequestHandlerRunner currently accepts the ServerFactory, which means removing it would require a signature change in the constructor of that class.

As it is, however, the ServerFactory now has a changed constructor, which is already a BC break.

As such, I'd argue we go full tilt, and make all the changes. We can release a 2.0 almost immediately, and let users know they can adopt it immediately if they were not extending any of these factories.

@gsteel gsteel changed the base branch from master to develop Nov 7, 2018

}
if ($this->enableCoroutine && method_exists(SwooleRuntime::class, 'enableCoroutine')) {
SwooleRuntime::enableCoroutine(true);

This comment has been minimized.

@gsteel

gsteel Nov 7, 2018

Contributor

SwooleRuntime::enableCoroutine() is no longer getting called anywhere. Where is this call best placed?
Reading the docs at https://wiki.swoole.com/wiki/page/965.html (And using some Google Translate magic), it's the kind of call you'd put in the entry point of your app (???) - I don't really understand the coroutine stuff, so it's a bit over my head, but as far as the HTTP server is concerned, coroutines are enabled by default inside the server according to the enable_coroutine server option here: https://wiki.swoole.com/wiki/page/949.html - I could very well be mistaken though…

This comment has been minimized.

@weierophinney

weierophinney Nov 7, 2018

Member

Different coroutine support entirely. 😄

The coroutine support in the HTTP server is around the events it triggers and listens to. The SwooleRuntime::enableCoroutine() method toggles the value at the runtime engine level, which controls coroutine support for TCP and socket operations (think: calling a remote API from within your code; using TCP operations to talk to a Redis server (as Predis does); etc.).

I'd argue this should go into the HttpServerFactory, as that's our entry point for creating the server instance and governing how it will run; that's also where we have access to the configuration values.

This comment has been minimized.

@gsteel

gsteel Nov 7, 2018

Contributor

Thanks for clearing that up for me! Looking at the deleted ServerFactory, it was getting enable_coroutine from the http server options array - which defaults to true in the server (I think). So, would it make sense to move the enable_coroutine flag up to the top level so that it's effect is unambiguous - i.e. I've set it to false instead of deleting the option in one of my test projects, turned off coroutine support in the server and had task workers throw a wobbly.

'zend-expressive-swoole' => [
    'enable_coroutine' => false, // Enable Runtime Coroutine
    'swoole-http-server' => [
        'host' => 'whatever.com',
        'port' => 80,
        'mode' => SWOOLE_PROCESS,
        'options' => [
            // Various HTTP Server Options…
            'enable_coroutine' => true, // Server Coroutine Support
        ],
    ],
],

This comment has been minimized.

@weierophinney

weierophinney Nov 8, 2018

Member

Yeah, this would make sense - and is another reason to mark this for a new major version, as users who were enabling the flag in the 1.0 series would need to change the option used.

This comment has been minimized.

@gsteel

gsteel Nov 8, 2018

Contributor

OK. I've just updated the server factory and the docs where it's mentioned

$swooleServer->on('workerstart', [$this, 'onWorkerStart']);
$swooleServer->on('request', [$this, 'onRequest']);
$swooleServer->start();
if ($this->isRunning()) {

This comment has been minimized.

@gsteel

gsteel Nov 7, 2018

Contributor

There was previously no check for whether the server was already running or not. Was this intentional for some reason that I've missed?

This comment has been minimized.

@weierophinney

weierophinney Nov 7, 2018

Member

Not intentional. We covered it for reload and stop operations, but missed it for start operations. Good catch!

@gsteel

Feedback on this is most welcome. Documentation is not my strong suit 😅

@weierophinney

Hi, @gsteel - this is looking great!

I'll merge #42 shortly, and then also push an update to the develop branch doing the following:

  • Creating a docs/book/v2/ structure based on the current v1/ tree.
  • Update the composer.json to indicate that dev-develop will alias to 2.0.x-dev instead of 1.1.x-dev.

When I've done that, I'll ping you, so that you can rebase and make your doc changes in the v2 tree.

There are a few minor CS issues, but the majority of the changes I'm requesting are documentation related, and outlined below.

Show resolved Hide resolved docs/book/v1/async-tasks.md Outdated
return $server;
}
}
```

This comment has been minimized.

@weierophinney

weierophinney Nov 8, 2018

Member

The previous section and the above confused me a bit, particularly after reading the Swoole project async task docs. My understanding is that in order to enable tasks, you need to do exactly one thing: register a listener on the 'task' event. Registering a 'finish' event is optional, per that document, and only necessary if your 'task' listener returns a value; you can also pass a callback to task() that will execute on task completion.

The task listener itself figures out what to do with the data, and could potentially perform actions based on both the data passed to it, as well as the result of processing, which means we can make the example more succinct and opinionated.

As an example, we could do something that composes both a logger and a message notifier, and it might look something like this:

use Psr\EventDispatcher\MessageInterface;
use Psr\EventDispatcher\MessageNotifierInterface;
use Psr\Log\LoggerInterface;

class TaskWorker
{
    private $notifier;
    private $logger;

    public function __construct(LoggerInterface $logger, MessageNotifierInterface $notifier)
    {
        $this->logger = $logger;
        $this->notifier = $notifier;
    }

    public function __invoke($server, $taskId, $fromId, $data)
    {
        if (! $data instanceof MessageInterface) {
            $this->logger->error('Invalid data provided to task worker: {type}', ['type' => is_object($data) ? get_class($data) : gettype($data)]);
            return;
        }
        $this->logger->info('Starting work on task {taskId} using data: {data}', ['taskId' => $taskId, 'data' => json_encode($data)]);
        try {
            $this->notifier->notify($data);
        } catch (\Throwable $e) {
            $this->logger->error('Error processing task {taskId}: {error}', ['taskId' => $taskId, 'error' => $e->getTraceAsString()]);
        }
    }
}

$server->on('task', $container->get(TaskWorker::class));

This makes it more clear, at least to me, what you should be passing to on('task'), and what that callback's responsibilities are.

One thing I'm unclear about is whether Swoole allows registering multiple task listeners. If it doesn't, we need to document that, so folks don't try registering multiple times. If it does, what's the behavior? Does each get the same $data? What gets passed to finish in that situation? We should document those things here.

This comment has been minimized.

@gsteel

gsteel Nov 8, 2018

Contributor

I haven't finished reading this just yet - but on('Finish') is required in my experience - I get a fatal error anytime I omit that handler. I'm pretty confident that swoole does not allow multiple listeners - I've tested this with some events, but I'll go away and check the same holds true for task listeners etc.

This comment has been minimized.

@gsteel

gsteel Nov 8, 2018

Contributor

$server->task() signature is $server->task(mixed $userData, int $destinationWorkerId = -1) - The only way of processing tasks is in $server->on('Task', $callable) - Just been testing this out and reading up here: https://wiki.swoole.com/wiki/page/134.html

Show resolved Hide resolved docs/book/v1/how-it-works.md Outdated
Show resolved Hide resolved docs/book/v1/intro.md Outdated
Show resolved Hide resolved src/HttpServerFactory.php
Show resolved Hide resolved test/SwooleRequestHandlerRunnerFactoryTest.php Outdated
Show resolved Hide resolved test/SwooleRequestHandlerRunnerFactoryTest.php Outdated
Show resolved Hide resolved test/SwooleRequestHandlerRunnerFactoryTest.php Outdated

weierophinney added a commit to gsteel/zend-expressive-swoole that referenced this pull request Nov 8, 2018

Move documentation changes for zendframework#40 to v2 docs
Moves the documentation changes made in zendframework#40 to the v2 documentation
tree, and restores the v1 docs to their original state.

Also introduces the migration chapter, and adds both the migration and
async-task chapters to the TOC.
@weierophinney

This comment has been minimized.

Member

weierophinney commented Nov 8, 2018

@gsteel I updated the composer.json, and created the v2 docs tree. I then grabbed your branch, rebased it off of latest develop, and pushed the documentation changes to the correct tree. I've now pushed to your branch, so you can continue with the other changes. Make sure you pull from your remote and rebase!

@gsteel

This comment has been minimized.

Contributor

gsteel commented Nov 8, 2018

@weierophinney Thanks for the help with the example code - I've had another go at the docs when you have a chance to review. Cheers 🍻

Show resolved Hide resolved src/HttpServerFactory.php Outdated
@gsteel

This comment has been minimized.

Contributor

gsteel commented Nov 12, 2018

Hey @weierophinney - I think I'm done here now 😄

weierophinney added a commit to weierophinney/zend-expressive-swoole that referenced this pull request Nov 13, 2018

Ensure SSL support can be enabled
Per zendframework#44, while we allowed passing the SSL key and certificate as options
for the Swoole HTTP server, we were not allowing protocol options that
enabled SSL support. Specifically, you need to pass the union of
`SWOOLE_SSL` and one of either `SWOOLE_SOCK_TCP` or `SWOOLE_SOCK_TCP6`;
prior to this patch, the union types were not allowed.

This patch backports a test and test assets from zendframework#40, authored by
@gsteel, so that we can fix this in the version 1 series.
@azjezz

azjezz approved these changes Nov 13, 2018

@weierophinney

This comment has been minimized.

Member

weierophinney commented Nov 13, 2018

@gsteel — I rebased, as I'd done a bugfix patch earlier to master that fixes the SSL support, based on this patch. I have also edited the documentation, and added CHANGELOG entries.

Thanks again for the stellar patch!

@weierophinney weierophinney merged commit c9fc1d0 into zendframework:develop Nov 13, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@gsteel

This comment has been minimized.

Contributor

gsteel commented Nov 13, 2018

No worries! Thanks for all the help. There's one fly in the ointment I think which I'll take a look at as soon as possible, but with these changes, specifically the http server factory, the symfony cli commands for start/stop/reload operate in isolation - as soon as you call stop on a daemonized server, the container whips up another http server on the same port and host due to the dependency on the handler runner - AFAIK, this is a warning, i.e. 'Can’t bind to port'. The commands still operate correctly (Haven't yet had time to verify that properly), but the notice/warning feels kinda off. Thought I'd better let you know before 2.0 happens!

@weierophinney

This comment has been minimized.

Member

weierophinney commented Nov 13, 2018

@gsteel I was kind of wondering about that as I edited the documentation. I'll give it a try, and if it's problematic, we can either remove the stop/reload commands, or figure out a way to re-work them.

@gsteel gsteel deleted the gsteel:server-instance-factory branch Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment