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

CommandLineUtils causes MqttClient to be instantiated due to m_internal_client #725

Closed
sparrowt opened this issue Jun 6, 2024 · 2 comments · Fixed by #726
Closed

CommandLineUtils causes MqttClient to be instantiated due to m_internal_client #725

sparrowt opened this issue Jun 6, 2024 · 2 comments · Fixed by #726
Labels
bug This issue is a bug. p3 This is a minor priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.

Comments

@sparrowt
Copy link

sparrowt commented Jun 6, 2024

Describe the bug

While running samples/pub_sub/basic_pub_sub I noticed that very early on when it calls Utils::parseSampleInputPubSub(...) -> CommandLineUtils() is causing an MqttClient to be created which causes the default event loop group to be made resulting in 8 new AwsEventLoop threads:
image

This all seems a bit unnecessary just in order to parse the command line args?

I wonder if this is caused by m_internal_client here in CommandLineUtils.h.

I'm not sure what that is for and searching for it shows no usages, just the single header occurrence.

Commenting out that line and rebuilding resolves the issue - the event loop threads aren't created until later on when Aws::Iot::MqttClient() is explicitly created.

Is m_internal_client meant to be there?

Expected Behavior

No MqttClient or event loop created until explicitly requested.

Current Behavior

MqttClient & event loop threads created during CommandLineUtils construction.

Reproduction Steps

Build & run samples/pub_sub/basic_pub_sub

Possible Solution

Remove m_internal_client assuming it is indeed unused?

Additional Information/Context

No response

SDK version used

39e497d

Environment details (OS name and version, etc.)

Windows 11 23H2 (unlikely to be relevant)

@sparrowt sparrowt added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 6, 2024
@jmklix
Copy link
Member

jmklix commented Jun 6, 2024

Thanks for pointing this out. This might have been used in the samples at some point, but it looks unused as of now. Testing before merging this PR: #726

@jmklix jmklix added pending-release This issue will be fixed by an approved PR that hasn't been released yet. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Jun 6, 2024
@jmklix jmklix linked a pull request Jun 6, 2024 that will close this issue
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. p3 This is a minor priority issue pending-release This issue will be fixed by an approved PR that hasn't been released yet.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants