Refactor unwrap to improve error handling #937

BLYKIM · 2024-12-23T07:36:32Z

Close: #899

codecov · 2024-12-23T07:45:23Z

Codecov Report

Attention: Patch coverage is 30.76923% with 18 lines in your changes missing coverage. Please review.

Project coverage is 77.18%. Comparing base (ffef4d1) to head (c2392e1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/peer.rs	0.00%	5 Missing ⚠️
src/storage.rs	0.00%	4 Missing ⚠️
src/graphql/export.rs	0.00%	3 Missing ⚠️
src/main.rs	0.00%	3 Missing ⚠️
src/graphql/statistics.rs	0.00%	1 Missing ⚠️
src/lib.rs	75.00%	1 Missing ⚠️
src/publish/implement.rs	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #937      +/-   ##
==========================================
+ Coverage   77.17%   77.18%   +0.01%     
==========================================
  Files          32       32              
  Lines       25723    25722       -1     
==========================================
+ Hits        19851    19854       +3     
+ Misses       5872     5868       -4

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

sophie-cluml · 2024-12-23T07:53:04Z

src/graphql/statistics.rs

-                    *iter_next_values.get_mut(idx).unwrap() = item;
+                    *iter_next_values
+                        .get_mut(idx)
+                        .expect("value is always exist") = item;


I would like to suggest fixing grammar: is always exist -> always exists

sophie-cluml · 2024-12-23T08:09:38Z

src/lib.rs

@@ -166,7 +166,7 @@ extern crate proc_macro;
 #[proc_macro_derive(ConvertGraphQLEdgesNode, attributes(graphql_client_type))]
 pub fn derive_from_graphql_client_autogen(input: TokenStream) -> TokenStream {
    derive_from_graphql_client_autogen_2(input.into())
-        .unwrap()
+        .expect("valid input struct")


Could you handle this with panic! macro, instead of expect please? The file lib.rs is the code for proc-macro , which runs against the Giganto codebase, during compile time only, to generate code from our custom macro. And this part can panic, when the Giganto codebase uses the custom macro incorrectly. My personal idea is that the message could be something like "ConvertGraphQLEdgesNode macro is not correctly used for {input}"

sophie-cluml · 2024-12-23T08:11:34Z

src/publish/implement.rs

@@ -13,14 +13,16 @@ pub trait RequestStreamMessage {

 impl RequestStreamMessage for RequestHogStream {
    fn channel_key(&self, sensor: Option<String>, record_type: &str) -> Result<Vec<String>> {
+        let sensor = sensor
+            .ok_or_else(|| anyhow!("Failed to generate hog channel key, sensor is required."))?;


I think we can use supervised model as the term. Or any other terms that refers to the supervised engine, that can be used in public space.

sophie-cluml · 2024-12-23T08:37:27Z

Could you check the commit message title's length?

sehkone · 2024-12-24T07:17:44Z

src/graphql/export.rs

-                        *iter_next_values.get_mut(min_index).unwrap() = item;
+                        *iter_next_values
+                            .get_mut(min_index)
+                            .expect("min_index's vector value always exists") = item;


I don't think this message is clear enough. I suggest:

"The index is one of the actual elements in the vector, so it is always valid."

sehkone · 2024-12-24T07:38:24Z

src/graphql/statistics.rs

@@ -224,8 +224,7 @@ fn gen_statistics(
        for idx in next_candidate {
            if let Some(iter) = stats_iters.get_mut(idx) {
                if let Some(item) = iter.next() {
-                    // replace new value (value is always exist)
-                    *iter_next_values.get_mut(idx).unwrap() = item;
+                    *iter_next_values.get_mut(idx).expect("value always exists") = item;


We need to specify the reason as clearly as possible, for example:

"`next_candidate` is generated while iterating over `iter_next_value`, so any index of the former is always valid within the latter."

sehkone · 2024-12-24T07:42:16Z

src/main.rs

@@ -313,7 +313,7 @@ async fn main() -> Result<()> {
        if is_reboot || is_power_off || is_reload_config {
            loop {
                {
-                    let retain_flag = retain_flag.lock().unwrap();
+                    let retain_flag = retain_flag.lock().expect("Mutex lock is safe");


Could you explain why this is safe?

If the message in expect about why the retain_flag is always safe needs to change, please also change the message about the running_flag in the retain_periodically function as well.

sehkone · 2024-12-24T07:43:44Z

src/publish.rs

@@ -528,8 +528,7 @@ where
            info!("start hog's publish stream : {:?}", record_type);
        }
        NodeType::Crusher => {
-            // crusher's policy Id always exists.
-            let id = msg.id().unwrap();
+            let id = msg.id().expect("crusher's policy Id always exists");


Could you explain why this always exists?

The id is a method that gets the value of the id field of the RequestCrusherStream structure that cruhser sends to giganto.
pub struct RequestCrusherStream { pub start: i64, pub id: String, pub src_ip: Option<IpAddr>, pub dst_ip: Option<IpAddr>, pub sensor: Option<String>, }

When crusher sends the value of that structure to giganto, it always includes the id value, so the value always exists.

Apart from this issue, the return value was Option because we used to call one method, source_id, to get the source or id values from the RequestStreamMessage trait, but now we have separated them into sensor and id. Id always has a value, so I think we can remove Option from the return value of the id method of the RequestStreamMessage trait in the future.

sehkone · 2024-12-24T07:44:59Z

src/storage/migration.rs

@@ -196,7 +196,7 @@ where
            continue;
        };
        let new_key = StorageKey::builder()
-            .start_key(&netflow_raw_event.sensor().unwrap())
+            .start_key(&netflow_raw_event.sensor().expect("always exists"))


Could you explain why this is safe?

The sensor is a method to get the value of the source field of Netflow5BeforeV23 and Netflow9BeforeV23.
pub struct Netflow5BeforeV23 { pub source: String, ..... } pub struct Netflow9BeforeV23 { pub source: String, ..... }

In earlier versions of giganto, before the structure of netflow5 and netflow9 was changed, saving Netflow5BeforeV23 and Netflow9BeforeV23 to the DB was done in the following order

Receive netflow5, netflow9 from REproduce. (via ingest)

Extract the source from the certificate in REproduce and store the value in the source field of each structure.

Store the final struct values to the DB.

Due to the above processing order, the source value always exists, and so do the results of the sensor that fetch them.

sehkone · 2024-12-27T07:56:42Z

src/main.rs

@@ -313,7 +313,7 @@ async fn main() -> Result<()> {
        if is_reboot || is_power_off || is_reload_config {
            loop {
                {
-                    let retain_flag = retain_flag.lock().unwrap();
+                    let retain_flag = retain_flag.lock().expect("Mutex lock is safe because it only guards a simple boolean flag without complex operations.");


I don't think this message sufficiently explains why we can always expect safe results.

Initially, I used the “always safe” message in expect because the Mutex only protects a simple boolean flag, which makes it unlikely to panic within the lock scope.
However, after considering the possibility of other parts of the system panicking while holding the lock, I decided to replace expect with unwrap_or_else.
In case of poisoning, the value is now safely recovered, and a warning message is logged to aid debugging.

sophie-cluml · 2024-12-31T06:02:57Z

src/storage.rs

@@ -1038,7 +1041,10 @@ pub async fn retain_periodically(
                }
                info!("Database cleanup completed.");
                {
-                    let mut running_flag = running_flag.lock().unwrap();
+                    let mut running_flag = running_flag.lock().unwrap_or_else(|e| {
+                        warn!("Poisoned Mutex. Recovering the boolean flag.");


I think we can use AtomicBool type instead of Mutex for the running_flag. I think we can avoid potential panicking from the mutex behavior, and achieve simpler code.

Thank you for the suggestion!

sophie-cluml · 2025-01-03T04:00:07Z

@BLYKIM Could you check the line length of the commit content?

- Refactor `Mutex<bool>` to `AtomicBool` to prevent potential panics and enhance performance.

BLYKIM requested review from sehkone and sophie-cluml December 23, 2024 07:36

sophie-cluml reviewed Dec 23, 2024

View reviewed changes

BLYKIM force-pushed the bly/remove_unwrap branch from 62a8574 to 7bf02b3 Compare December 24, 2024 05:01

BLYKIM changed the title ~~Refactor unwrap usage to improve error handling and prevent panic~~ Refactor unwrap to improve error handling Dec 24, 2024

BLYKIM force-pushed the bly/remove_unwrap branch from 7bf02b3 to 5afa77b Compare December 24, 2024 05:05

sehkone reviewed Dec 24, 2024

View reviewed changes

sehkone requested a review from kimhanbeom December 24, 2024 07:32

sehkone reviewed Dec 24, 2024

View reviewed changes

BLYKIM force-pushed the bly/remove_unwrap branch from 5afa77b to 0b7ae35 Compare December 27, 2024 07:16

sehkone reviewed Dec 27, 2024

View reviewed changes

BLYKIM force-pushed the bly/remove_unwrap branch from 0b7ae35 to 60747ac Compare December 30, 2024 23:32

sophie-cluml reviewed Dec 31, 2024

View reviewed changes

BLYKIM force-pushed the bly/remove_unwrap branch from 60747ac to eab96b0 Compare December 31, 2024 07:01

sophie-cluml approved these changes Dec 31, 2024

View reviewed changes

kimhanbeom approved these changes Jan 2, 2025

View reviewed changes

sehkone approved these changes Jan 3, 2025

View reviewed changes

Refactor unwrap to improve error handling

c2392e1

- Refactor `Mutex<bool>` to `AtomicBool` to prevent potential panics and enhance performance.

BLYKIM force-pushed the bly/remove_unwrap branch from eab96b0 to c2392e1 Compare January 3, 2025 04:27

sophie-cluml merged commit b096c59 into main Jan 3, 2025
9 of 10 checks passed

sophie-cluml deleted the bly/remove_unwrap branch January 3, 2025 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor unwrap to improve error handling #937

Refactor unwrap to improve error handling #937

BLYKIM commented Dec 23, 2024

codecov bot commented Dec 23, 2024 •

edited

Loading

sophie-cluml Dec 23, 2024

sophie-cluml Dec 23, 2024

sophie-cluml Dec 23, 2024 •

edited

Loading

sophie-cluml commented Dec 23, 2024

sehkone Dec 24, 2024

sehkone Dec 24, 2024

sehkone Dec 24, 2024

kimhanbeom Dec 24, 2024

sehkone Dec 24, 2024

kimhanbeom Dec 24, 2024 •

edited

Loading

sehkone Dec 24, 2024

kimhanbeom Dec 24, 2024 •

edited

Loading

sehkone Dec 27, 2024

BLYKIM Dec 30, 2024

sophie-cluml Dec 31, 2024 •

edited

Loading

BLYKIM Dec 31, 2024

sophie-cluml commented Jan 3, 2025

Refactor unwrap to improve error handling #937

Refactor unwrap to improve error handling #937

Conversation

BLYKIM commented Dec 23, 2024

codecov bot commented Dec 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophie-cluml Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

sophie-cluml commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimhanbeom Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimhanbeom Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophie-cluml Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sophie-cluml commented Jan 3, 2025

codecov bot commented Dec 23, 2024 •

edited

Loading

sophie-cluml Dec 23, 2024 •

edited

Loading

kimhanbeom Dec 24, 2024 •

edited

Loading

kimhanbeom Dec 24, 2024 •

edited

Loading

sophie-cluml Dec 31, 2024 •

edited

Loading