[ISSUE #5171]✨Add error handlingor uninitialized DefaultMQProducer in send_to_queue_with_callback_timeout

[WaterWhisperer]

Which Issue(s) This PR Fixes(Closes)

Fixes #5171

Brief Description

How Did You Test This Change?

Summary by CodeRabbit

• Bug Fixes
  • Improved stability by replacing panic behavior with proper error handling, ensuring the application returns meaningful error messages instead of crashing when initialization fails.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[rocketmq-rust-bot]

🔊@WaterWhisperer 🚀Thanks for your contribution🎉！

💡CodeRabbit(AI) will review your code first🔥！

Note

🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥.

[coderabbitai]

Walkthrough

This PR replaces an unsafe unwrap() call with guarded error handling in the send_to_queue_with_callback_timeout method. When DefaultMQProducerImpl is uninitialized (None), the method now returns a RocketMQError instead of panicking.

Changes

Cohort / File(s)	Summary
Error handling improvement rocketmq-client/src/producer/default_mq_producer.rs	Replaced unwrap() on optional DefaultMQProducerImpl with a guard clause that returns RocketMQError::not_initialized when the implementation is None

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

• [ISSUE #5165]✨Add error handlingor uninitialized DefaultMQProducer in fetch_publish_message_queues #5252 — Addresses similar error handling for uninitialized DefaultMQProducerImpl in the same file with comparable pattern replacement
• [ISSUE #5168]✨Add error handlingor uninitialized DefaultMQProducer in send_oneway #5313 — Applies the same unwrap-to-guard pattern in send_oneway method of the producer
• [ISSUE #5167]✨Add error handlingor uninitialized DefaultMQProducer in send_with_callback_timeout #5297 — Replaces unwrap calls in other send_*_with_callback_timeout methods with RocketMQError::not_initialized returns

Suggested labels

enhancement✨, Difficulty level/Easy, approved, auto merge

Suggested reviewers

• SpaceXCN
• mxsm
• TeslaRustor

Poem

🐰 A guardian clause hops into place,
No more unwrap's panic-stricken face,
Errors returned with grace and care,
The producer now handles None with flair!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding error handling for uninitialized DefaultMQProducer in send_to_queue_with_callback_timeout, which aligns with the code changes replacing unwrap() with proper error handling.
Linked Issues check	✅ Passed	The PR implements the requested enhancement from issue #5171 by replacing panic-prone unwrap() with proper error handling that returns RocketMQError::not_initialized() for uninitialized DefaultMQProducer.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the single file and function specified in the linked issue, with no unrelated modifications present.

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

rocketmq-client/src/producer/default_mq_producer.rs (1)

1203-1231: Critical: Test validates the wrong method.

This test is named request_with_callback_not_initialized and calls request_with_callback (Line 1222), but the PR actually fixes send_to_queue_with_callback_timeout (Line 787). The test does not verify the fix introduced in this PR, leaving the change untested.

🔎 Proposed fix to test the correct method

     #[tokio::test]
-    async fn request_with_callback_not_initialized() {
+    async fn send_to_queue_with_callback_timeout_not_initialized() {
         // Arrange
         let mut producer = DefaultMQProducer {
             client_config: Default::default(),
             producer_config: Default::default(),
             default_mqproducer_impl: None,
         };
         let msg = Message {
             topic: "test-topic".into(),
             flag: 0,
             properties: Default::default(),
             body: None,
             compressed_body: None,
             transaction_id: None,
         };
         let callback = |_msg: Option<&dyn MessageTrait>, _err: Option<&dyn std::error::Error>| {
             // no-op
         };
-        let result = producer.request_with_callback(msg, callback, 1000).await;
+        let mq = MessageQueue::default();
+        let result = producer.send_to_queue_with_callback_timeout(msg, mq, callback, 1000).await;
         assert!(result.is_err());
         let err = result.unwrap_err();
         match err {
             RocketMQError::NotInitialized(reason) => {
                 assert!(reason.contains("not initialized"), "unexpected error message: {reason}");
             }
             other => panic!("Unexpected error: {other:?}"),
         }
     }

🧹 Nitpick comments (1)

rocketmq-client/src/producer/default_mq_producer.rs (1)

798-802: Consider applying the same error handling pattern to remaining methods.

While this PR correctly fixes send_to_queue_with_callback_timeout, many other methods still use unwrap() on default_mqproducer_impl.as_mut() and will panic if the producer is uninitialized. For consistency and robustness, consider replacing these with the ok_or() pattern used in the fix.

Examples of methods still using unwrap():

• Line 800: send_oneway_to_queue
• Line 948: send_batch
• Line 1015: send_batch_with_callback
• And approximately 15+ other methods throughout the file

This ensures all methods provide clear error messages instead of panicking, improving the overall API ergonomics.

Also applies to: 948-948, 1013-1015

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ceb282 and 6035dde.

📒 Files selected for processing (1)

• rocketmq-client/src/producer/default_mq_producer.rs

🧰 Additional context used

🧬 Code graph analysis (1)

rocketmq-client/src/producer/default_mq_producer.rs (1)

rocketmq-error/src/unified.rs (1)

• not_initialized (521-523)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Build & Test (windows-latest)
• GitHub Check: Build & Test (macos-latest)
• GitHub Check: Build & Test (ubuntu-latest)
• GitHub Check: Code Coverage
• GitHub Check: auto-approve

🔇 Additional comments (1)

rocketmq-client/src/producer/default_mq_producer.rs (1)

785-787: LGTM! Proper error handling replaces unsafe unwrap.

The change correctly replaces the panic-prone unwrap() with guarded error handling that returns a descriptive error when DefaultMQProducerImpl is uninitialized. This matches the PR objective and is consistent with similar error handling patterns elsewhere in the file.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 37.10%. Comparing base (853d39a) to head (6035dde).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ocketmq-client/src/producer/default_mq_producer.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5366      +/-   ##
==========================================
- Coverage   37.17%   37.10%   -0.08%     
==========================================
  Files         800      800              
  Lines      108077   108308     +231     
==========================================
+ Hits        40182    40187       +5     
- Misses      67895    68121     +226     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rocketmq-rust-bot]

LGTM - All CI checks passed ✅