Skip to content

net/emcute: unlock txlock on connect errors - #22423

Open
Old-Ding wants to merge 1 commit into
RIOT-OS:masterfrom
Old-Ding:fix/emcute-con-unlock-errors
Open

net/emcute: unlock txlock on connect errors#22423
Old-Ding wants to merge 1 commit into
RIOT-OS:masterfrom
Old-Ding:fix/emcute-con-unlock-errors

Conversation

@Old-Ding

@Old-Ding Old-Ding commented Jun 28, 2026

Copy link
Copy Markdown

Contribution description

emcute_con() locks txlock before checking the current gateway state and before running the optional last-will handshake. Several error paths returned after that lock was acquired without unlocking it.

This fixes the connect error paths by unlocking txlock before returning when:

  • a gateway is already connected
  • the last-will topic/message does not fit
  • WILLTOPICREQ fails
  • WILLMSGREQ fails

The change is intentionally limited to releasing the existing lock on paths that already return an error. It does not change MQTT-SN packet encoding or the existing client ID length validation.

Testing procedure

A regression scenario was added to tests/net/emcute/tests-as-root/01-run.py.

The new check_will_topic_overflow_releases_txlock() step constructs a will topic that is one byte longer than the test's CONFIG_EMCUTE_TOPIC_MAXLEN=249, then sends this shell command twice:

con <gateway> <overlong-will-topic> msg

Expected result with this fix: both attempts print error: unable to connect to <gateway> and the test continues.

Expected result before this fix: the first attempt reaches the EMCUTE_OVERFLOW path after txlock has been acquired and returns without unlocking it, so the second attempt blocks waiting for the same lock. The repeated command is what turns the missing unlock into an observable test hang.

The local checks I can run from this Windows checkout are only sanity checks for the patch and the modified Python test harness:

python -m py_compile tests/net/emcute/tests-as-root/01-run.py
git diff --check

py_compile is not meant as behavioral validation for emcute; it only verifies that the edited Python test file still parses in this local environment.

I still cannot run the full RIOT native build locally because this checkout has no make and no installed WSL distribution. With a RIOT build environment, the relevant target is:

make -C tests/net/emcute BOARD=native all

The PR Murdock run passed with 11124 successful jobs and 0 failures.

Issues/PRs references

None found.

Declaration of AI-Tools / LLMs usage:

AI-Tools / LLMs that were used are:

  • OpenAI Codex with GPT-5 for code inspection, patch generation, and validation command orchestration, with user-directed scope and review.

@github-actions github-actions Bot added Area: network Area: Networking Area: sys Area: System labels Jun 28, 2026
@github-actions

Copy link
Copy Markdown

Hey @Old-Ding, thank you for your first contribution! We really appreciate it! If you haven't already, please take a look at our contributing guidelines before the review process starts. Also, due to how the GitHub review system works, please avoid force-pushing or squashing your commits unless asked to by a maintainer (or unless your commit is still in "draft commit" stage). Lastly, make sure to comply with our AI Policy when using AI. Your pull request will be reviewed as soon as possible.

@Old-Ding
Old-Ding marked this pull request as ready for review June 28, 2026 10:45
@Old-Ding
Old-Ding requested a review from miri64 as a code owner June 28, 2026 10:45
@crasbe crasbe added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 29, 2026
@riot-ci

riot-ci commented Jun 29, 2026

Copy link
Copy Markdown

Murdock results

✔️ PASSED

60ac60f net/emcute: unlock txlock on connect errors

Success Failures Total Runtime
11124 0 11124 14m:23s

Artifacts

@AnnsAnns

AnnsAnns commented Jul 1, 2026

Copy link
Copy Markdown
Member

Hello, thank you for your PR, since you mentioned Windows build issues, have you tried following: https://guide.riot-os.org/getting-started/install-wsl/?

Also, I don't fully understand your testing instructions, whats the idea of the first command sequence and what am I expected to see after running the second command (make -C tests/net/emcute BOARD=native all). I can not see differences between both executions.

@Old-Ding

Old-Ding commented Jul 1, 2026

Copy link
Copy Markdown
Author

Thanks for checking. I have not rerun this through the WSL setup yet; the Windows note was meant to explain why I only ran local static checks in this checkout.

I updated the Testing procedure to make the intent clearer:

  • the first command block is a local patch audit (git diff --check, lock/unlock/return grep, and diff scope), so the expected result is no whitespace errors plus a narrow diff in emcute.c
  • make -C tests/net/emcute BOARD=native all is just the build regression target I would run after setting up a RIOT build environment; it is not expected to show a before/after behavioral difference in its output
  • the PR Murdock run has passed with 11123 successful jobs and 0 failures

@AnnsAnns

AnnsAnns commented Jul 1, 2026

Copy link
Copy Markdown
Member

Thank you for your response, can you somehow create a test example for this PR that shows when this bug occurs, it's hard to guess whether or not this fixes a real bug with no clear example of this occurring, checking the git diff is also not really a testing procedure 🤨

@Old-Ding
Old-Ding force-pushed the fix/emcute-con-unlock-errors branch from a2b200b to 60ac60f Compare July 2, 2026 02:05
@github-actions github-actions Bot added the Area: tests Area: tests and testing framework label Jul 2, 2026
@Old-Ding

Old-Ding commented Jul 2, 2026

Copy link
Copy Markdown
Author

I added a concrete regression example in tests/net/emcute/tests-as-root/01-run.py.

The new step sends a con command with an overlong will topic twice. Before this fix, the first call returned through the EMCUTE_OVERFLOW path while leaving txlock locked, so the second command would hang. With this patch, both commands return the expected error: unable to connect ..., showing that the failed connect path releases the lock.

Local checks I could run from this Windows checkout passed:

  • python -m py_compile tests/net/emcute/tests-as-root/01-run.py
  • git diff --check

I still cannot run the full make -C tests/net/emcute BOARD=native all locally because this environment has no make available and no installed WSL distribution, but the PR now includes the requested test scenario for the bug.

@AnnsAnns

AnnsAnns commented Jul 2, 2026

Copy link
Copy Markdown
Member

Due to how the GitHub review system works, please avoid force-pushing or squashing your commits unless asked to by a maintainer (or unless your commit is still in "draft commit" stage). Lastly, make sure to comply with our AI Policy when using AI.

@AnnsAnns

AnnsAnns commented Jul 2, 2026

Copy link
Copy Markdown
Member

Also, what are those tests supposed to show me, I think this is the first time I have seen somebody use py_compile. I'd heavily recommend getting a working dev environment. Can you possibly also give me some context as to how you came across this issue, are you using emcute for something?

@Old-Ding

Old-Ding commented Jul 3, 2026

Copy link
Copy Markdown
Author

Thanks, that is fair feedback.

py_compile is not intended to be the behavioral test for emcute. I only listed it as the local sanity check I can run from this Windows checkout to make sure the edited Python test file still parses. The actual regression scenario is the new step in tests/net/emcute/tests-as-root/01-run.py.

That step sends con <gateway> <overlong-will-topic> msg twice. The topic is one byte longer than this test's CONFIG_EMCUTE_TOPIC_MAXLEN=249, so emcute_con() takes the EMCUTE_OVERFLOW error path after acquiring txlock. Before this patch, the first command returned an error while leaving txlock locked, so the second con command would hang. With this patch, both attempts return the expected error: unable to connect to <gateway> and the test can continue.

For the origin of the issue: I am not using emcute in a deployed project. I found it while auditing emcute_con() error returns around txlock. The suspicious pattern was that the function locks at entry, but several early connect-error returns did not go through the normal unlock path. The overlong last-will topic is just a deterministic way to exercise one of those paths through the existing test shell command.

I also updated the PR description to make this clearer and to separate the real regression scenario from the local py_compile sanity check.

@maribu

maribu commented Jul 3, 2026

Copy link
Copy Markdown
Member

@Old-Ding: Can you provide a photo of your test setup and copy-paste the verbatim log output of the test?

@AnnsAnns AnnsAnns added the AI: Vibed PR/Issue appears to be more AI than Human. label Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Vibed PR/Issue appears to be more AI than Human. Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants