prooph / event-store-client

PHP Event Store Client Implementation
BSD 3-Clause "New" or "Revised" License
113 stars 31 forks source link

Connection process cannot be "waited" #145

Closed Nek- closed 4 years ago

Nek- commented 4 years ago

Expected behavior

wait($connection->connectAsync());

TCP Connection ready after this. Any further instruction yield to be waited as well.

Actual behavior

The connection is not even created, at this state either nothing has been done or discovering endpoints, which is part of the connection.

This means that any statement that comes after (and is yielded!) will not be finished and wait because the library has an internal "retry" mechanism when the connection is not ready.

I think this is a broken usage of Amphp that must be fixed. WDYT?

I open this issue after posting on stackoverflow and some hours on trying to debug the thing.

prolic commented 4 years ago

wait is helper wrapper around Loop::run that stops the loop as soon as the passed promise is resolved. What you are trying to do is a bad idea in general. When being in a sync app, why don't you use the http-client?

prolic commented 4 years ago

Even if you would make it work somehow, imagine this:

wait($connection->connectAsync());
sleep(10);

What happened? You just destroyed your connection. Once the loop is stopped (this is what wait does) the heartbeats are not longer handled and all incoming messages from the event-store server are not handled.

prolic commented 4 years ago

This is how a connection is established:

https://github.com/prooph/event-store-client/blob/master/src/Internal/EventStoreConnectionLogicHandler.php#L247-L251

As you can see here, the command to establish the connection is enqueued and the promised is resolved.

See https://github.com/prooph/event-store-client/blob/master/src/Internal/EventStoreConnectionLogicHandler.php#L312-L340

The actual establishment of the connection is deferred on the loop and handled on the next tick.

This means if you call wait($connection->connectAsync()); your connection might not yet be ready. In an async application that's not a problem at all, you can already write to or subscribe from streams, those actions will be performed asynchronously as soon as the connection is established.

You can look at the unit tests and examples folders for a lot of working examples.

Nek- commented 4 years ago

Yes, my point is that the connection promise should not be resolve at this state because it's not connected yet.

Btw ofc a sleep is not a good idea, it was just a super weird not working test. I expect my sync process to end before 200ms (which would be already super slow) so I assume the connection can wait a little bit.

prolic commented 4 years ago

Yes, my point is that the connection promise should not be resolve at this state because it's not connected yet.

This is a wrong assumption. The promise will be resolved, once the command to establish a connection has been enqueued. Again, if you are working in a standard-php application, which is sync and blocking (a.k.a. you use wait), stick to the HTTP client implementation (https://github.com/prooph/event-store-http-client/).

What you are describing here is the actual desired behavior.

Nek- commented 4 years ago

I should be able to connect using high performance way (direct socket) that's how we use mysql and postgresql, I want to do same with eventstore.

Maybe my implementation is not the best (I already have ideas to make it better).

But one thing I'm sure about, if I yield for connection, then connection must be established and ready. Amphp is not designed to be yolo asyc, it has pattern, yield to wait for it is one of them.

If you do not follow patterns, then I can't use your library. Because this is a bug to me. (actually even for you since you have to check if it's ready and requeues things, which is not normal in a standard amphp process, unless the connection is super slow to establish, witch is not the case here)

Feel free to contact me, I'm open to trying some fixes here.

prolic commented 4 years ago

Again, this is not a bug. The connection doesn't need to be there once connectAsync is waited for. See also the original .NET client which this is a port from: https://github.com/EventStore/EventStore/blob/master/src/EventStore.ClientAPI/Internal/EventStoreConnectionLogicHandler.cs#L136-L137

If you want to wait until the connection is ready, you can do that by resolving a custom deferred using the onConnected handler. Something like this should work:

$deferred = new Deferred();
$connection->onConnected(function () use ($deferred): void {
    $deferred->resolve(null);
});
wait($deferred->promise());