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

update: closing MQTT connection on application shutdown #16

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

Conversation

yagotlima
Copy link

Possible solution for Issue #15. I'm not sure if MqttService is the best place for this but it's working.

constructor(
@Inject(MQTT_CLIENT_INSTANCE) private readonly client: Client,
) {}

onModuleDestroy() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires adding app.enableShutdownHooks() to main.ts.

IMHO, the proper way to deal with the closing the client(s) is doing it outside of nest-mqtt:

  • nest-mqtt should export (if it does not already does so) the client object;
  • users should add app.enableShutdownHooks() to their main.ts;
  • create a class for OnModuleDestroy();
  • add that class to the providers array in app.module.ts.

I don’t really think there is anything to do in nest-mqtt (unless client object is not yet exported). However, I don’t use net-mqtt anymore, as I needed to many stuff to implement in order to make it work as I need it to work, therefore I re-implemented it within my codebase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's not exporting the client yet.

I agree that this line is kind of misplaced. Also delegating this responsibility to users is more flexible in case you don't want client#end to be called for whatever reason.

I'll submit a second PR exposing the client object and documenting how to properly close it's connection. This way, whichever approach is decided to be the best can be merged and the other closed.

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

Successfully merging this pull request may close these issues.

2 participants