toptobes commented on PR #462:
URL:
https://github.com/apache/cassandra-nodejs-driver/pull/462#issuecomment-4502122465
> I think, the main reason why it broke in the past, is that `this.host` and
`this.connection` are a nullable type, (they are **supposed to be null** in
certain circumstainces like refreshing), but function calls like
`this._setHealthListeners(this.host, this.connection);` are not treating them
as nullable. Imagine if this is TypeScript, it would complain that
`_setHealthListeners` expects `Host, Connection` as argument while this call
passes `Host?, Connection?`. So, aside from the `_refreshInProgress` flag to
make sure only one refresh is happening at one time, I think we should still
make sure we are using `this.host` and `this.connection` as nullable. That
means ` this._setHealthListeners(this.host, this.connection);` should be
>
> ```
> if (this.host && this.connection){
> this._setHealthListeners(this.host, this.connection);
> }
> ```
>
> And other places that access `this.host` and `this.connection` should also
has null guards, like
[this](https://github.com/toptobes/cassandra-nodejs-driver/blob/37e314ff8d9d9f0974d2185e9d6b43f1349447e4/lib/control-connection.js#L408).
What do you think?
As far as I can tell, the nullability is meant to be a transient state and
trying to impose these silent null checks may just cause more subtle bugs than
it solves? I wouldn't be against explicit null checks in `_setHealthListeners`
as an assertion, but simply ignoring setting the listeners could be problematic
in an ideal world we may want to use an explicit state machine but that
seems pretty out of scope for this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
