basis-company / nats.php

nats jetstream client for php
116 stars 29 forks source link

Info message unavailable #80

Closed ro0NL closed 2 months ago

ro0NL commented 2 months ago

There's an edge case error when the connection is dropped:

Cannot assign null to property Basis\Nats\Connection::$infoMessage of type Basis\Nats\Message\Info

/app/vendor/basis-company/nats/src/Connection.php:202
/app/vendor/basis-company/nats/src/Connection.php:127
/app/vendor/basis-company/nats/src/Client.php:207
/app/vendor/basis-company/nats/src/Client.php:127
/app/vendor/basis-company/nats/src/Consumer/Consumer.php:105

But the API expects non-nullable, and im wondering if it should be refactored a bit :)

nekufa commented 2 months ago

interesting case, looks like we need to check that info message was received or throw an exception :) btw, have no idea how to cover that with tests)

ro0NL commented 2 months ago

i think the type error should be prevented either way yes.

we could do:

$this->infoMessage = $this->getMessage($config->timeout) ?? throw new \RuntimeException('Stream outage');
// assert() is useless at this point, and no-op in prod ;)

but the other question is what happened?

i noticed getMessage() doesnt check for false coming from stream_get_line, which would indicate failure

perhaps it should be checked strictly and perform a retry once out-the-box, to then throw.

ro0NL commented 2 months ago

It's an edge case. Let's move on.