-
-
Notifications
You must be signed in to change notification settings - Fork 19
[WIP] Cleanly stop() loop when awaiting promise without fiber
#65
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
Conversation
WyriHaximus
commented
Oct 29, 2022
This is going to be a fun one to test out before we can ship it to production 😅
WyriHaximus
commented
Dec 4, 2022
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
commented
Dec 5, 2022
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 😅
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
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
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
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
commented
Mar 15, 2023
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
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
commented
Mar 20, 2023
@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
commented
Mar 20, 2023
@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
commented
May 5, 2023
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
testSyncPublishWithAsyncConsumealways 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
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
commented
May 12, 2023
@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());
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 mixawait()andLoop::run()calls. Theawait()call may currently suspend theLoop::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