hivemq / hivemq-mqtt-client

HiveMQ MQTT Client is an MQTT 5.0 and MQTT 3.1.1 compatible and feature-rich high-performance Java client library with different API flavours and backpressure support
https://hivemq.github.io/hivemq-mqtt-client/
Apache License 2.0
872 stars 161 forks source link

Change event loop to use daemon instead of non-daemon threads #638

Open ViToni opened 3 months ago

ViToni commented 3 months ago

The class io.netty.util.concurrent.DefaultThreadFactory defaults to non-daemon thread if not explicitly configured. Threads created with defaults by this ThreadFactory might prevent unaware applcations from quitting.

Type of Change

Checklist

cla-bot[bot] commented 3 months ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @ViToni on file. In order for us to review and merge your code, please sign our Contributor License Agreement to get yourself added. You'll find the CLA and more information here: https://github.com/hivemq/hivemq-community/blob/master/CONTRIBUTING.adoc#contributor-license-agreement

ViToni commented 3 months ago

@cla-bot check

cla-bot[bot] commented 3 months ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @ViToni on file. In order for us to review and merge your code, please sign our Contributor License Agreement to get yourself added. You'll find the CLA and more information here: https://github.com/hivemq/hivemq-community/blob/master/CONTRIBUTING.adoc#contributor-license-agreement

cla-bot[bot] commented 3 months ago

The cla-bot has been summoned, and re-checked this pull request!

ViToni commented 3 months ago

@cla-bot check

cla-bot[bot] commented 3 months ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @ViToni on file. In order for us to review and merge your code, please sign our Contributor License Agreement to get yourself added. You'll find the CLA and more information here: https://github.com/hivemq/hivemq-community/blob/master/CONTRIBUTING.adoc#contributor-license-agreement

cla-bot[bot] commented 3 months ago

The cla-bot has been summoned, and re-checked this pull request!

ViToni commented 3 months ago

@cla-bot check

cla-bot[bot] commented 3 months ago

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @ViToni on file. In order for us to review and merge your code, please sign our Contributor License Agreement to get yourself added. You'll find the CLA and more information here: https://github.com/hivemq/hivemq-community/blob/master/CONTRIBUTING.adoc#contributor-license-agreement

cla-bot[bot] commented 3 months ago

The cla-bot has been summoned, and re-checked this pull request!

ViToni commented 3 months ago

@SgtSilvio @pglombardo Could you please check your CLA signing process? I signed the documents about 3 times now and the state hasn't changed, yet.

sauroter commented 3 months ago

Hi @ViToni, thank you for creating the PR. Something in the CLA process seems to be stuck. I reached out internally to see what is going on. Sorry for the inconvenience.

sauroter commented 3 months ago

@cla-bot check

cla-bot[bot] commented 3 months ago

The cla-bot has been summoned, and re-checked this pull request!

SgtSilvio commented 2 months ago

This is a behavioral change. A simple example: if you have a main function that publishes a message via the MQTT client asynchronously, and you don’t block the main thread until the message is fully published/acknowledged, and the MQTT client threads are all daemon thread, the process will exit before successfully sending the message.

Regarding the issue: The MQTT client's threads should stop automatically when not needed anymore. If I remember correctly, the threads are not released while the client still has a session. This might be the cause for your issue. We should rather take a look if this can be improved.

ViToni commented 2 months ago

@sauroter Thanks for caring.

ViToni commented 2 months ago

This is a behavioral change. A simple example: if you have a main function that publishes a message via the MQTT client asynchronously, and you don’t block the main thread until the message is fully published/acknowledged, and the MQTT client threads are all daemon thread, the process will exit before successfully sending the message.

Aggreed, this might be a behavioral change, if any user would really rely on this behavior. However I'm asuming users would have more complex applications than just sending individual messages which means there are more dependencies involved, each with their dedicated lifecyle.

IMHO a user himself should take care of, when his application quits and not depend on one of its dependencies to be responsible. In this case having non-daemon threads makes it actually more difficult to quit the application in case of e.g. a bug or wrong usage of the hivemq-mqtt-client library (such as in #633)

Regarding the issue: The MQTT client's threads should stop automatically when not needed anymore. If I remember correctly, the threads are not released while the client still has a session. This might be the cause for your issue. We should rather take a look if this can be improved.

In #633 we saw that when using cleanSession=false threads can still continue to run. This PR is not meant as a solution or workaround for the behavior seen, but rather a means take such issue less impactful. In our case we cannot use cleanSession=false and have seen two concurrent sessions (hopefully the right wording) which interfere with each other (means both sessions try to establish a connection) and both sessions have non-daemon threads.

But finally this PR can only be regarded as a suggestion.