Proper handling thread creation failure #3063
Conversation
- Now freeing allocated memory. - Throwing general exception, not the code for the EE to process exception.
WalkthroughThe pull request introduces consistent error handling improvements across multiple AzureRTOS platform-specific implementations for I2C and OneWire device interfaces. The changes focus on refining thread creation error handling by ensuring proper memory cleanup and using a more generalized error code ( Changes
Possibly related PRs
Suggested labels
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
targets/AzureRTOS/SiliconLabs/_nanoCLR/nanoFramework.Device.OneWire/nf_dev_onewire_nanoFramework_Device_OneWire_OneWireHost.cpp (1)
474-478: LGTM! Proper cleanup on thread creation failure.The changes correctly handle thread creation failure by freeing the allocated stack memory before returning the error code, preventing potential memory leaks.
However, consider adding a debug log message to help with troubleshooting thread creation failures in development builds.
if (status != TX_SUCCESS) { // failed to create the thread +#if !defined(BUILD_RTM) + // Log the actual error code in debug builds + DEBUG_PRINT("Failed to create 1-Wire thread. Status: %d\n", status); +#endif // free stack memory platform_free(workingThreadStack); NANOCLR_SET_AND_LEAVE(CLR_E_FAIL); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp(1 hunks)targets/AzureRTOS/SiliconLabs/_nanoCLR/nanoFramework.Device.OneWire/nf_dev_onewire_nanoFramework_Device_OneWire_OneWireHost.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: nf-interpreter (Check_Code_Style)
- GitHub Check: nf-interpreter (Nightly build) (Check_Build_Options)
- GitHub Check: nf-interpreter (Check_Build_Options)
🔇 Additional comments (4)
targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
586-590: LGTM! Improved error handling for thread creation failure.The changes properly handle resource cleanup by freeing allocated memory before returning the error code when thread creation fails.
targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp (1)
583-587: LGTM! Consistent error handling improvement.The changes align with the SiliconLabs implementation, ensuring proper resource cleanup on thread creation failure.
targets/AzureRTOS/SiliconLabs/_nanoCLR/nanoFramework.Device.OneWire/nf_dev_onewire_nanoFramework_Device_OneWire_OneWireHost.cpp (2)
Line range hint
8-9: Verify thread stack size consistency.The thread stack size should be consistent with similar operations across the codebase.
✅ Verification successful
Thread stack size is appropriate
The
ONEWIRE_THREAD_STACK_SIZEof 256 bytes is consistent with similar peripheral operations in the codebase, particularly matching the I2C operations thread stack size.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check thread stack size definitions across the codebase # Look for thread stack size definitions to ensure consistency # Search for thread stack size definitions rg 'THREAD_STACK_SIZE.*[0-9]+' # Search for similar OneWire operations thread definitions rg 'OneWire.*STACK_SIZE'Length of output: 6431
474-478: Verify consistent thread creation error handling across the codebase.Let's ensure that other thread creation instances follow the same pattern of proper cleanup on failure.
✅ Verification successful
Thread creation error handling is consistent across the codebase
The reviewed code follows the established pattern used throughout the codebase for thread creation error handling:
- Proper memory cleanup on thread creation failure
- Consistent error codes (CLR_E_FAIL for thread creation, CLR_E_OUT_OF_MEMORY for allocation)
- Resource cleanup in correct order (thread deletion followed by stack memory freeing)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find similar thread creation patterns to verify consistent error handling # Look for tx_thread_create calls and check their error handling # Search for thread creation patterns ast-grep --pattern 'tx_thread_create($$$)' # Search for potential memory leaks in thread creation error handling rg -A 5 'tx_thread_create.*TX_SUCCESS'Length of output: 7735
Script:
#!/bin/bash # Get more context around thread creations to see error handling patterns # Looking at both before and after thread creation # OneWire implementation rg -B 10 -A 10 "tx_thread_create.*WaitingTask" targets/AzureRTOS/SiliconLabs/_nanoCLR/nanoFramework.Device.OneWire/ # I2C implementations rg -B 10 -A 10 "tx_thread_create.*WorkingThread" targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/ rg -B 10 -A 10 "tx_thread_create.*WorkingThread" targets/AzureRTOS/SiliconLabs/_nanoCLR/System.Device.I2c/ # WiFi implementation rg -B 10 -A 10 "tx_thread_create.*wifiConnectThread" targets/AzureRTOS/ST/_common/drivers/wifi/Length of output: 424
Script:
#!/bin/bash # Search for thread stack allocation and error handling patterns # Look for stack allocations and surrounding context rg -B 5 -A 15 "ThreadStack.*=.*platform_malloc" # Look for thread creation error handling patterns rg -B 5 -A 10 "if.*status.*!=.*TX_SUCCESS" # Get content of specific files we know have thread creation cat "targets/AzureRTOS/SiliconLabs/_nanoCLR/nanoFramework.Device.OneWire/nf_dev_onewire_nanoFramework_Device_OneWire_OneWireHost.cpp" | grep -A 10 -B 10 "workingThreadStack" cat "targets/AzureRTOS/ST/_nanoCLR/System.Device.I2c/sys_dev_i2c_native_System_Device_I2c_I2cDevice.cpp" | grep -A 10 -B 10 "WorkingThreadStack"Length of output: 59683
(cherry picked from commit 3fd2ac0)
Description
Motivation and Context
How Has This Been Tested?
Screenshots
Types of changes
Checklist
Summary by CodeRabbit