[#164] Prevent leaking of NetClient instances

[sophokles73]

The NetClientImpl used by MqttClientImpl to connect to a server requires its close() method to be invoked to release some resources which would otherwise prevent it from being GCed properly.

The MqttClientImpl's disconnect() method has been changed to invoke the NetClientImpl.close() method instead of just the close() method of the NetSocketImpl instance that represents the connection to the MQTT server.

[vietj]

thanks, in master we might do differently as we changed the lifecycle for clients

[vietj]

you can have a look at latest SQL client that creates an un-managed NetClient (using VertxInternal) and registers a close hook for itself. We should do the same for MQTT client in master.

[vietj]

https://github.com/eclipse-vertx/vertx-sql-client/blob/master/vertx-pg-client/src/main/java/io/vertx/pgclient/impl/PgConnectionFactory.java shows how the client can be created and closed

[vietj]

This shows how the a client can register a close hook https://github.com/eclipse-vertx/vertx-sql-client/blob/master/vertx-pg-client/src/main/java/io/vertx/pgclient/impl/PgPoolImpl.java#L52

[vietj]

I think however for 3.9 we should simply do as you proceeded

[sophokles73]

@vietj are you proposing to create two separate PRs, one for master and one for 3.9?

[vietj]

indeed, both versions won't address client closing the same way

[sophokles73]

@vietj The MqttClientImpl allows the same client instance to be used to re-connect to a server after its disconnect() method has been invoked. This behavior is not explicitly specified by the MqttClient JavaDocs but closing the underlying NetClientImpl as part of the disconnect() method would prevent client code from taking advantage of this behavior. I therefore suggest to add a close() method to the MqttClient interface instead, which closes the connection and the NetClientImpl. Client code can then decide if it wants to re-use an MqttClient instance or not. Using the close() method would then make sure that the discarded MqttClient can be cleany GCed. WDYT?

[vietj]

I think it's good

On Fri, Aug 28, 2020 at 2:00 PM Kai Hudalla notifications@github.com wrote:

@vietj https://github.com/vietj The MqttClientImpl allows the same client instance to be used to re-connect to a server after its disconnect() method has been invoked. This behavior is not explicitly specified by the MqttClient JavaDocs but closing the underlying NetClientImpl as part of the disconnect() method would prevent client code from taking advantage of this behavior. I therefore suggest to add a close() method to the MqttClient interface instead, which closes the connection and the NetClientImpl. Client code can then decide if it wants to re-use an MqttClient instance or not. Using the close() method would then make sure that the discarded MqttClient can be cleany GCed. WDYT?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/vert-x3/vertx-mqtt/pull/166#issuecomment-682486576, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXDCUTS3GIPHEZ6GPSRGDSC6L63ANCNFSM4QKMTAGQ .

[ppatierno]

@sophokles73 are you going to do the latest changes you mentioned on this PR or on a new one?

[sophokles73]

@ppatierno yes, that is my intention. I also think that we can use the same approach for both master and 3.9. WDYT?

[vietj]

3.9 does not have a CloseFuture and it will not work the same way. However in 3.9 you can also implement the automatic close if that's used within a Verticle, the code will be slighly different and you cannot really backport the commit.

[vietj]

I'll handle this myself in master and back port this change as is in 3.9 for 3.9.3

[vietj]

So actually I think what is missing is a close() operation on the MQTT client that would close the underlying NetClient.

Closing the NetClient on the MQTT client will prevent reconnecting unless we make the client not final anymore and create an instance when it's needed.

[vietj]

I went in a slight different way, since we are using a single connection per client, we instantiate the client when connect() is called. When the connection is closed, the net client is closed too. I've changed to set the ctx and connection field to null when the close handler is called. See https://github.com/vert-x3/vertx-mqtt/pull/169

[vietj]

See https://github.com/vert-x3/vertx-mqtt/pull/170