Return error if calling function will block host task#419
Open
Return error if calling function will block host task#419
Conversation
This will prevent applications from hanging forever when calling a blocking function from the host task.
📝 WalkthroughWalkthroughThis pull request adds runtime guards across multiple NimBLE methods to prevent blocking or discovery operations from executing within the NimBLE host task context. A new task-tracking mechanism records the host task identifier at startup, enabling callers to detect and reject unsafe operations with early returns and error logging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEClient.cpp`:
- Around line 683-685: The guard that rejects calls from the host task
(NimBLEUtils::inHostTask()) is currently placed at the start of getService(),
preventing access to cached services; move that inHostTask() check so it runs
immediately before the first call to retrieveServices() (i.e., only when a cache
miss would trigger a blocking retrieval), and leave the early-path that returns
a cached service intact so host-task callbacks can still return previously
discovered services from the cache via getService().
In `@src/NimBLEDevice.cpp`:
- Around line 879-881: The host task ID stored in NimBLEUtils::m_hostTaskHandle
is set before calling nimble_port_run() but never cleared when nimble_port_run()
returns, causing inHostTask() to give false positives after deinit; fix by
resetting NimBLEUtils::m_hostTaskHandle to an invalid value (e.g., 0/null) once
nimble_port_run() returns and before/after calling nimble_port_freertos_deinit()
so the stored task handle is not reused by unrelated code.
In `@src/NimBLERemoteCharacteristic.cpp`:
- Around line 158-160: The guard that blocks host-task context is placed too
early in getDescriptor(), preventing cached descriptors from being returned;
move the NimBLEUtils::inHostTask() check so it happens immediately before
calling retrieveDescriptors() (i.e., keep early cache-scan logic in
getDescriptor() intact, then if a discovery is required call the inHostTask()
guard before invoking retrieveDescriptors()); update references in
getDescriptor() and ensure setNotify() still treats a found cached CCCD
correctly (no change to setNotify() logic required).
In `@src/NimBLERemoteService.cpp`:
- Around line 78-80: The current early-return in getCharacteristic when
NimBLEUtils::inHostTask() is true prevents returning cached characteristics;
instead, only prevent calling the blocking retrieveCharacteristics(). Modify
getCharacteristic so it still scans the existing cache (the loop that checks
cached characteristics) even when inHostTask() is true, but conditionally skip
or gate the call to retrieveCharacteristics() (and log if desired) when
inHostTask() to avoid blocking the host task; reference functions/methods:
getCharacteristic(), retrieveCharacteristics(), and NimBLEUtils::inHostTask() to
locate and implement this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc557eb3-0366-4adb-8ddc-ea45aa79f46a
📒 Files selected for processing (8)
src/NimBLEClient.cppsrc/NimBLEDevice.cppsrc/NimBLERemoteCharacteristic.cppsrc/NimBLERemoteService.cppsrc/NimBLERemoteValueAttribute.cppsrc/NimBLEScan.cppsrc/NimBLEUtils.cppsrc/NimBLEUtils.h
Comment on lines
+683
to
+685
| if (NimBLEUtils::inHostTask()) { | ||
| NIMBLE_LOGE(LOG_TAG, "getService cannot be called from host task"); | ||
| return nullptr; |
There was a problem hiding this comment.
Arrr, cached services should still be reachable.
This now rejects getService() before the cache scan on Lines 688-693. That breaks host-task callbacks that only need a previously discovered service even though no blocking call would occur. Move the guard to just before the first retrieveServices() call so only cache misses are rejected.
⚓ Proposed fix
NimBLERemoteService* NimBLEClient::getService(const NimBLEUUID& uuid) {
NIMBLE_LOGD(LOG_TAG, ">> getService: uuid: %s", uuid.toString().c_str());
- if (NimBLEUtils::inHostTask()) {
- NIMBLE_LOGE(LOG_TAG, "getService cannot be called from host task");
- return nullptr;
- }
for (auto& it : m_svcVec) {
if (it->getUUID() == uuid) {
NIMBLE_LOGD(LOG_TAG, "<< getService: found the service with uuid: %s", uuid.toString().c_str());
return it;
}
}
+
+ if (NimBLEUtils::inHostTask()) {
+ NIMBLE_LOGE(LOG_TAG, "getService cannot discover from host task");
+ return nullptr;
+ }
size_t prevSize = m_svcVec.size();
if (retrieveServices(&uuid)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEClient.cpp` around lines 683 - 685, The guard that rejects calls
from the host task (NimBLEUtils::inHostTask()) is currently placed at the start
of getService(), preventing access to cached services; move that inHostTask()
check so it runs immediately before the first call to retrieveServices() (i.e.,
only when a cache miss would trigger a blocking retrieval), and leave the
early-path that returns a cached service intact so host-task callbacks can still
return previously discovered services from the cache via getService().
Comment on lines
+879
to
881
| NimBLEUtils::m_hostTaskHandle = ble_npl_get_current_task_id(); | ||
| nimble_port_run(); // This function will return only when nimble_port_stop() is executed | ||
| nimble_port_freertos_deinit(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'm_hostTaskHandle|inHostTask\s*\(' srcRepository: h2zero/esp-nimble-cpp
Length of output: 5298
Arr, swab the deck—reset m_hostTaskHandle when the host task be walkin' the plank!
This bit o' code be stashin' the host-task ID at startup but never clairin' it when nimble_port_run() returns, ye scallywag! If th' NPL/RTOS be reusinn' that task handle later, unrelated callers might start trippin' inHostTask() checks and gettin' spurious early rejects after deinit.
⚓ Proposed fix
void NimBLEDevice::host_task(void* param) {
NIMBLE_LOGI(LOG_TAG, "NimBLE Started!");
NimBLEUtils::m_hostTaskHandle = ble_npl_get_current_task_id();
nimble_port_run(); // This function will return only when nimble_port_stop() is executed
+ NimBLEUtils::m_hostTaskHandle = nullptr;
nimble_port_freertos_deinit();
} // host_task📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| NimBLEUtils::m_hostTaskHandle = ble_npl_get_current_task_id(); | |
| nimble_port_run(); // This function will return only when nimble_port_stop() is executed | |
| nimble_port_freertos_deinit(); | |
| NimBLEUtils::m_hostTaskHandle = ble_npl_get_current_task_id(); | |
| nimble_port_run(); // This function will return only when nimble_port_stop() is executed | |
| NimBLEUtils::m_hostTaskHandle = nullptr; | |
| nimble_port_freertos_deinit(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEDevice.cpp` around lines 879 - 881, The host task ID stored in
NimBLEUtils::m_hostTaskHandle is set before calling nimble_port_run() but never
cleared when nimble_port_run() returns, causing inHostTask() to give false
positives after deinit; fix by resetting NimBLEUtils::m_hostTaskHandle to an
invalid value (e.g., 0/null) once nimble_port_run() returns and before/after
calling nimble_port_freertos_deinit() so the stored task handle is not reused by
unrelated code.
Comment on lines
+158
to
+160
| if (NimBLEUtils::inHostTask()) { | ||
| NIMBLE_LOGE(LOG_TAG, "getDescriptor cannot be called from host task"); | ||
| return nullptr; |
There was a problem hiding this comment.
Arrr, let cached descriptors pass; only block discovery.
This return fires before the cache scan on Lines 167-172, so even an already discovered descriptor now looks missing from host-task callbacks. Downstream, setNotify() on Lines 249-257 treats that nullptr as “CCCD not found” and returns true, so a cached CCCD becomes a silent no-op. Move the guard to just before retrieveDescriptors().
⚓ Proposed fix
NimBLERemoteDescriptor* NimBLERemoteCharacteristic::getDescriptor(const NimBLEUUID& uuid) const {
NIMBLE_LOGD(LOG_TAG, ">> getDescriptor: uuid: %s", uuid.toString().c_str());
- if (NimBLEUtils::inHostTask()) {
- NIMBLE_LOGE(LOG_TAG, "getDescriptor cannot be called from host task");
- return nullptr;
- }
NimBLEUUID uuidTmp{uuid};
NimBLETaskData taskData(const_cast<NimBLERemoteCharacteristic*>(this));
NimBLEDescriptorFilter filter{nullptr, &uuidTmp, &taskData};
for (const auto& dsc : m_vDescriptors) {
if (dsc->getUUID() == uuid) {
filter.dsc = dsc;
goto Done;
}
}
+
+ if (NimBLEUtils::inHostTask()) {
+ NIMBLE_LOGE(LOG_TAG, "getDescriptor cannot discover from host task");
+ goto Done;
+ }
if (!retrieveDescriptors(&filter) || filter.dsc) {
goto Done;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLERemoteCharacteristic.cpp` around lines 158 - 160, The guard that
blocks host-task context is placed too early in getDescriptor(), preventing
cached descriptors from being returned; move the NimBLEUtils::inHostTask() check
so it happens immediately before calling retrieveDescriptors() (i.e., keep early
cache-scan logic in getDescriptor() intact, then if a discovery is required call
the inHostTask() guard before invoking retrieveDescriptors()); update references
in getDescriptor() and ensure setNotify() still treats a found cached CCCD
correctly (no change to setNotify() logic required).
Comment on lines
+78
to
+80
| if (NimBLEUtils::inHostTask()) { | ||
| NIMBLE_LOGE(LOG_TAG, "getCharacteristic cannot be called from host task"); | ||
| return nullptr; |
There was a problem hiding this comment.
Arrr, don’t sink cached characteristic lookups.
This guard runs before the cache scan on Lines 85-91, so host-task callbacks can no longer fetch an already discovered characteristic even though no blocking call would occur. Gate only retrieveCharacteristics() here.
⚓ Proposed fix
NimBLERemoteCharacteristic* NimBLERemoteService::getCharacteristic(const NimBLEUUID& uuid) const {
NIMBLE_LOGD(LOG_TAG, ">> getCharacteristic: uuid: %s", uuid.toString().c_str());
- if (NimBLEUtils::inHostTask()) {
- NIMBLE_LOGE(LOG_TAG, "getCharacteristic cannot be called from host task");
- return nullptr;
- }
NimBLERemoteCharacteristic* pChar = nullptr;
for (const auto& it : m_vChars) {
if (it->getUUID() == uuid) {
pChar = it;
NIMBLE_LOGD(LOG_TAG, "<< getCharacteristic: found in cache");
return pChar;
}
}
+
+ if (NimBLEUtils::inHostTask()) {
+ NIMBLE_LOGE(LOG_TAG, "getCharacteristic cannot discover from host task");
+ return nullptr;
+ }
if (retrieveCharacteristics(&uuid, &pChar) && pChar == nullptr) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLERemoteService.cpp` around lines 78 - 80, The current early-return
in getCharacteristic when NimBLEUtils::inHostTask() is true prevents returning
cached characteristics; instead, only prevent calling the blocking
retrieveCharacteristics(). Modify getCharacteristic so it still scans the
existing cache (the loop that checks cached characteristics) even when
inHostTask() is true, but conditionally skip or gate the call to
retrieveCharacteristics() (and log if desired) when inHostTask() to avoid
blocking the host task; reference functions/methods: getCharacteristic(),
retrieveCharacteristics(), and NimBLEUtils::inHostTask() to locate and implement
this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This will prevent applications from hanging forever when calling a blocking function from the host task.
Summary by CodeRabbit