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

Documentation for timer API and clean up unneeded timer API #102

Merged
merged 5 commits into from
Apr 28, 2017

Conversation

clue
Copy link
Member

@clue clue commented Apr 26, 2017

The timer API is currently entirely undocumented and offers several duplicate methods and accessors not really required. Arguably, this makes the timer API appear more difficult than it actually is. The loop accessors also causes an unneeded cyclic dependency which hinders garbage collection.

This PR adds documentation for the existing timer API and then subsequently removes unneeded methods and the cyclic dependency. Note that while this PR does remove parts of the existing API, it does not add any new APIs or otherwise modify the existing APIs. While this does contain some BC breaks, many consumers will not be affected by this due to the duplicate methods. This PR ensures to provide some upgrade guides to those affected by the BC breaks.

Performance improvement is not a major motivation here, but shows a negligible 3-5% improvement anyway (running examples 92 and 94).

If you want to review, consider also looking at the individual commits.

Builds on top of #99 and #100.

cancelTimer() and isTimerActive()

// old (methods invoked on timer instance)
$timer->cancel();
assert(!$timer->isActive());

// already supported before: invoke methods on loop instance
$loop->cancelTimer($timer);
assert(!$loop->isTimerActive($timer));

Use closure binding to add arbitrary data to timer

// old (limited setData() and getData() only allows single variable)
$name = 'Tester';
$timer = $loop->addTimer(1.0, function ($timer) {
    echo 'Hello ' . $timer->getData() . PHP_EOL;
});
$timer->setData($name);

// already supported before: closure binding allows any number of variables
$name = 'Tester';
$loop->addTimer(1.0, function () use ($name) {
    echo 'Hello ' . $name . PHP_EOL;
});

Remove getLoop() and suggest using closure binding instead

// old (getLoop() called on timer instance)
$loop->addTimer(0.1, function ($timer) {
    $timer->getLoop()->addTimer(0.1, function () {
        echo 'Hello';
    });
});

// already supported before: use closure binding as usual
$loop->addTimer(0.1, function () use ($loop) {
    $loop->addTimer(0.1, function () {
        echo 'Hello';
    });
});

@clue clue added this to the v0.5.0 milestone Apr 26, 2017
The mixed "data" attribute is mostly unused and can easily be replaced
by closure binding instead.

This also prepares the timer instance to become an immutable
data structure.
These can easily be replaced by Loop::cancelTimer() and
Loop::isTimerActive().

This change ensures that timers are now a plain immutable data structure
and offer no methods to change its state.

This also prepares the timer to remove the cyclic dependency with the
loop instance.
Thid can easily be replaced by using closure binding as documented.
Copy link
Member

@nrk nrk left a comment

Choose a reason for hiding this comment

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

These changes definitely make sense at this stage of ReactPHP. The current API defined by React\EventLoop\TimerInterface was motivated by an hypothetical transition to a full object-based approach that wrapped resources managed by the event loop, but this vision never really took shape for various reasons (even practical ones). Probably this was never discussed in the public or maybe just on IRC, this is something that happened at the very early stages of the project after all.

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

Successfully merging this pull request may close these issues.

None yet

4 participants