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

[WIP] Cleanly stop() loop when awaiting promise without fiber #65

Open
wants to merge 1 commit into
base: 4.x
Choose a base branch
from

Conversation

clue
Copy link
Member

@clue clue commented Oct 26, 2022

This changeset makes sure we cleanly stop() the loop when awaiting a promise without a fiber. This is especially important for test suites and other use cases where you would mix await() and Loop::run() calls. The await() call may currently suspend the Loop::run() call and leave it in an unpredictable state, possibly leaving socket resources or timers behind.

Builds on top of #15 and #32
Refs #27 and #50

@clue clue added bug Something isn't working new feature New feature or request labels Oct 26, 2022
@clue clue added this to the v4.1.0 milestone Oct 26, 2022
@WyriHaximus
Copy link
Member

This is going to be a fun one to test out before we can ship it to production 😅

@WyriHaximus
Copy link
Member

Interestingly enough I'm running into this while making the reactphp/socket#302 passes before opening it up for review. Will check our non-test environments and report back.

@WyriHaximus
Copy link
Member

Deployed it in a side project to see how it goes, after inspecting the code it shouldn't be an issue but I want to see it running for a while without causing issues first 😅

WyriHaximus added a commit to WyriHaximus-labs/socket that referenced this pull request Dec 5, 2022
This commit introduces the functionality required to build opportunistic TLS clients and servers with
ReactPHP. It does so by introducing a prefix to `tls://`, namely `opportunistic`, to create
`opportunistic+tls://example.com:5432` for example as the full URL. This will create an
`OpportunisticTlsConnectionInterface` (instead of a `ConnectionInterface`) that extends the
`ConnectionInterface` and exposes the `enableEncryption` method to enable TLS encryption at the
desired moment. Inside this PR is an example of a server and client negotiating when to enable TLS
and enable it when ready.

Depends on: reactphp/async#65
Opportunistic Security described in RFC7435: https://www.rfc-editor.org/rfc/rfc7435
External PR using the proposed changes in this commit: voryx/PgAsync#52
WyriHaximus added a commit to WyriHaximus-labs/socket that referenced this pull request Dec 7, 2022
This commit introduces the functionality required to build opportunistic TLS clients and servers with
ReactPHP. It does so by introducing a prefix to `tls://`, namely `opportunistic`, to create
`opportunistic+tls://example.com:5432` for example as the full URL. This will create an
`OpportunisticTlsConnectionInterface` (instead of a `ConnectionInterface`) that extends the
`ConnectionInterface` and exposes the `enableEncryption` method to enable TLS encryption at the
desired moment. Inside this PR is an example of a server and client negotiating when to enable TLS
and enable it when ready.

Depends on: reactphp/async#65
Opportunistic Security described in RFC7435: https://www.rfc-editor.org/rfc/rfc7435
External PR using the proposed changes in this commit: voryx/PgAsync#52
WyriHaximus added a commit to WyriHaximus-labs/socket that referenced this pull request Dec 7, 2022
This commit introduces the functionality required to build opportunistic TLS clients and servers with
ReactPHP. It does so by introducing a prefix to `tls://`, namely `opportunistic`, to create
`opportunistic+tls://example.com:5432` for example as the full URL. This will create an
`OpportunisticTlsConnectionInterface` (instead of a `ConnectionInterface`) that extends the
`ConnectionInterface` and exposes the `enableEncryption` method to enable TLS encryption at the
desired moment. Inside this PR is an example of a server and client negotiating when to enable TLS
and enable it when ready.

Depends on: reactphp/async#65
Opportunistic Security described in RFC7435: https://www.rfc-editor.org/rfc/rfc7435
External PR using the proposed changes in this commit: voryx/PgAsync#52
WyriHaximus added a commit to WyriHaximus-labs/socket that referenced this pull request Dec 7, 2022
This commit introduces the functionality required to build opportunistic TLS clients and servers with
ReactPHP. It does so by introducing a prefix to `tls://`, namely `opportunistic`, to create
`opportunistic+tls://example.com:5432` for example as the full URL. This will create an
`OpportunisticTlsConnectionInterface` (instead of a `ConnectionInterface`) that extends the
`ConnectionInterface` and exposes the `enableEncryption` method to enable TLS encryption at the
desired moment. Inside this PR is an example of a server and client negotiating when to enable TLS
and enable it when ready.

Depends on: reactphp/async#65
Opportunistic Security described in RFC7435: https://www.rfc-editor.org/rfc/rfc7435
External PR using the proposed changes in this commit: voryx/PgAsync#52
@tomas-novotny
Copy link

Hello, hows the testing going @WyriHaximus, is it ready for merging? It would helped us with fixing test suites for react/async: ^4.0.

@clue
Copy link
Member Author

clue commented Mar 15, 2023

It would helped us with fixing test suites for react/async: ^4.0.

@tomas-novotny I'm curious, can you give some details what specific use case this would help with? I consider this change a WIP that could potentially have a significant impact (see #27 for context), so I would like to get a better understanding how this would affect the larger ecosystem.

@tomas-novotny
Copy link

@clue I tried to come up with a simplified version of our tests that were failing without this change, but I was unable to do so.

On further investigation, I found that ^4.0 behaves a little differently than ^3.0, but actually better, so the problem was our wrong test.

So after fixing a few assertions and calling cancelTimer everything was fixed and now it passes our tests.

@WyriHaximus
Copy link
Member

@tomas-novotny Well actually, I've been running this in production on a low traffic project for nearly 4 months without issues. As far as I know because I do have a bunch of odd pod restarts but those could also have different reasons for restarting.

@tomas-novotny
Copy link

I managed to create some test cases that fail with reactphp/async:4.0.0.

https://gist.github.com/tomas-novotny/5abdbd4474c25e768c6eeb168c32cfc1

  • The testPublishConsume[1/2/3] test cases pass when you run them individually, but when you run them all, only the first one passes and the rest fail with various errors.

  • The testSyncPublishWithAsyncConsume always fails.

These PR changes fix these tests. I have not been able to fix them any other way.

Is there something I am doing wrong or some change that will make it work without these PR changes?

@clue
Copy link
Member Author

clue commented May 5, 2023

@tomas-novotny Thank you for looking into this and providing reproducible test scripts! Definitely a good start, though I would love to see if we could eventually trim this down to remove any external dependencies to get to the core of this.

The gist here seems to be that your test suite mixes await() and Loop::run() calls which usually should be avoided. This PR here ensures that await() cleanly stops the loop so a following Loop::run() works without issues, but the recommendation to not mix both styles still seems reasonable in either case. We're working on making the Loop opaque in a future ReactPHP v3 (https://github.com/orgs/reactphp/discussions/481), so I wonder what impact this would have on your test cases.

@tomas-novotny
Copy link

@clue Thank you for your hints how to fix my test suites.
I replaced Loop:run() with awaiting for a deferred promise and it works fine even without this PR changes.

+$deferred = new Deferred();

// failsafe to stop loop
$timers[] = $loop->addTimer(5, static function () use ($loop): void {
-    $loop->stop();
+    $deferred->reject();
});

// periodic check if all messages have been consumed
$timers[] = $loop->addPeriodicTimer(1, static function (TimerInterface $timer) use ($loop, $messages, &$consumedMessages): void {
    if (count($messages) !== count($consumedMessages)) {
        return;
    }

    $loop->cancelTimer($timer);
-   $loop->stop();
+   $deferred->resolve();
});

// finish consuming
await($consumer);

// run loop
-$loop->run();
+await($deferred->promise());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants