Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ViToni
Copy link

@ViToni ViToni commented Aug 14, 2024

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

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've written tests (if applicable) for all new methods and classes that I created.
  • I've added documentation as necessary so users can easily use and understand this feature/fix.

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.
Copy link

cla-bot bot commented Aug 14, 2024

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
Copy link
Author

ViToni commented Aug 19, 2024

@cla-bot check

Copy link

cla-bot bot commented Aug 19, 2024

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

Copy link

cla-bot bot commented Aug 19, 2024

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

@ViToni
Copy link
Author

ViToni commented Aug 19, 2024

@cla-bot check

Copy link

cla-bot bot commented Aug 19, 2024

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

Copy link

cla-bot bot commented Aug 19, 2024

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

@ViToni
Copy link
Author

ViToni commented Aug 19, 2024

@cla-bot check

Copy link

cla-bot bot commented Aug 19, 2024

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

Copy link

cla-bot bot commented Aug 19, 2024

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

@ViToni
Copy link
Author

ViToni commented Aug 26, 2024

@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
Copy link
Member

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
Copy link
Member

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Aug 30, 2024
Copy link

cla-bot bot commented Aug 30, 2024

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

@SgtSilvio
Copy link
Member

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
Copy link
Author

ViToni commented Sep 25, 2024

@sauroter Thanks for caring.

@ViToni
Copy link
Author

ViToni commented Sep 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants