fix(router): replace the occurrences of `gen_range` with a safer alternative #1908

anupj · 2023-08-09T13:08:48Z

Type of Change

Description

This is a bugfix for issue 1754.
I have replaced the instances of gen_range() with Uniform.sample.
This should reduce the risk of gen_range() throwing a panic!.

Additional Changes

This PR modifies the API contract
This PR modifies the database schema
This PR modifies application configuration/environment variables

Motivation and Context

This is a bugfix for issue 1754
Closes #1754.

How did you test it?

I did not write any new tests but I ran cargo test -p router --lib and all tests passed.

NOTE: I was not able to run all integration tests because my laptop is not setup for running integration tests.

Checklist

I formatted the code cargo +nightly fmt --all
I addressed lints thrown by cargo clippy
I reviewed submitted code
I added unit tests for my changes where possible
I added a CHANGELOG entry if applicable

lsampras · 2023-08-09T14:47:25Z

crates/router/src/routes/dummy_connector/utils.rs

@@ -15,8 +15,11 @@ use crate::{configs::settings, routes::AppState};

 pub async fn tokio_mock_sleep(delay: u64, tolerance: u64) {
    let mut rng = rand::thread_rng();
-    let effective_delay = rng.gen_range((delay - tolerance)..(delay + tolerance));
-    tokio::sleep(tokio::Duration::from_millis(effective_delay)).await
+    let effective_delay = Uniform::from((delay - tolerance)..(delay + tolerance));


can we use Uniform::try_from ?.

(although that also does not seem to help)
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=4a99d8a5b9a9604292cf7065e83e2866

Maybe it'll be fixed in future, we can use the safer API on our side in the meanwhile

@lsampras I started with try_from:
let timeout = Uniform::try_from(0..=settings.loop_interval)?;
But the compiler complains that:

the trait `std::convert::From<Infallible>` is not implemented for `error_stack::Report<ProcessTrackerError>`

We can't just implement From<Infallible> for error_stack::Report<ProcessTrackerError> because of Rusts orphan rule. If we want to get around this then we may have to add significant amount of workaround in the errors.rs file potentially changing the API contract.
Plus as you can see above it says to implement something that is Infallible that is unlikely to fail so we are not gaining much here. And another reason is that the from method just does type conversion here which is infallible; the actual random number work is done by the sample method which replaces the gen_range functionality and again doesn't return a Result.

Does that reasoning make sense?

@lsampras @SanchithHegde any thoughts on the comment above?

let timeout = Uniform::try_from(0..=settings.loop_interval).into_report().change_context(ProcessTrackerError::InvalidInput)?;

I believe this should fix it^

yeah the result is currently infallible but in the future it's not gonna be,
we won't have much benefit from adding it rn but we'll be benefitting from this when a new version of rand is released

@lsampras cool, thanks. That is good to know. I'll resubmit the PR.

I believe this still needs to be changed to Uniform::try_from()?

lsampras · 2023-08-09T14:47:58Z

crates/router/src/scheduler/consumer.rs

-    let timeout = rand::thread_rng().gen_range(0..=settings.loop_interval);
-    tokio::time::sleep(Duration::from_millis(timeout)).await;
+    let mut rng = rand::thread_rng();
+    let timeout = Uniform::from(0..=settings.loop_interval);


Uniform::try_from

anupj · 2023-08-16T06:35:13Z

@SanchithHegde I've updated the PR, let me know if there are any other changes required here. Thanks.

SanchithHegde

Other than that, looks good to me!

Thanks for the PR, @anupj!

SanchithHegde · 2023-08-16T16:51:17Z

crates/router/src/scheduler/producer.rs

+    let mut rng = rand::thread_rng();
+    let timeout = Uniform::try_from(0..=scheduler_settings.loop_interval)
+        .into_report()
+        .change_context(errors::ProcessTrackerError::InvalidInput)?;


~~I'd prefer this being named as InvalidConfiguration instead~~, I'd prefer raising ConfigurationError instead, since loop_interval is a scheduler configuration variable, and this would provide us better error messages. We can remove the InvalidInput error variant as well.

Is this PR blocked because of this?

@anupj Not exactly blocked by the error propagation, there's still one remaining usage of Uniform::from() that would have to be replaced.

#1908 (comment)

Let us know if you need any help.

Ok, I'll fix this today/tomorrow and update PR for review. Thanks.

@SanchithHegde I remember now why I didn't change this to try_from() because it will mean that we'll have to change the function signature for tokio_mock_sleep() to return a Result which could turn out to be a bigger change than is required for this PR as we haave to then change the fn signature where tokioo_mock_sleep() is called. Is that desirable? ?

Can you then add a TODO here & if possible create a new issue for this that can be picked up later?

SanchithHegde · 2023-08-16T16:51:54Z

crates/router/src/routes/dummy_connector/utils.rs

@@ -15,8 +15,11 @@ use crate::{configs::settings, routes::AppState};

 pub async fn tokio_mock_sleep(delay: u64, tolerance: u64) {
    let mut rng = rand::thread_rng();
-    let effective_delay = rng.gen_range((delay - tolerance)..(delay + tolerance));
-    tokio::sleep(tokio::Duration::from_millis(effective_delay)).await
+    let effective_delay = Uniform::from((delay - tolerance)..(delay + tolerance));


I believe this still needs to be changed to Uniform::try_from()?

SanchithHegde · 2023-09-09T17:41:06Z

@anupj Could you please address the merge conflicts?

anupj · 2023-09-11T10:27:44Z

@anupj Could you please address the merge conflicts?

Oh dear, looks like I might have accidentally closed the PR. Is it better to open a new PR to address the merge issues?

SanchithHegde · 2023-09-11T19:26:39Z

Oh dear, looks like I might have accidentally closed the PR. Is it better to open a new PR to address the merge issues?

Either is fine.

anupj requested review from a team as code owners August 9, 2023 13:08

lsampras requested changes Aug 9, 2023

View reviewed changes

pixincreate assigned anupj Aug 9, 2023

SanchithHegde added A-framework Area: Framework C-bug Category: Bug C-refactor Category: Refactor S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Aug 9, 2023

SanchithHegde changed the title ~~fix(router): fix for issue 1754~~ fix(router): replace the occurrences of gen_range with a safer alternative Aug 9, 2023

anupj requested a review from a team as a code owner August 12, 2023 07:14

SanchithHegde reviewed Aug 16, 2023

View reviewed changes

pixincreate requested review from SanchithHegde and a team August 31, 2023 05:52

bernard-eugine added the ageing >2weeks Created > 2 weeks label Sep 1, 2023

anupj closed this Sep 10, 2023

anupj force-pushed the main branch from 1ed7271 to 0792605 Compare September 10, 2023 06:26

SanchithHegde mentioned this pull request Sep 11, 2023

fix(router/scheduler): replace the occurrences of gen_range with a safer alternative #2126

Merged

15 tasks

SanchithHegde removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments ageing >2weeks Created > 2 weeks labels Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router): replace the occurrences of `gen_range` with a safer alternative #1908

fix(router): replace the occurrences of `gen_range` with a safer alternative #1908

anupj commented Aug 9, 2023 •

edited by SanchithHegde

lsampras Aug 9, 2023

anupj Aug 9, 2023 •

edited

anupj Aug 10, 2023 •

edited

lsampras Aug 11, 2023

anupj Aug 12, 2023

SanchithHegde Aug 16, 2023

lsampras Aug 9, 2023

anupj commented Aug 16, 2023

SanchithHegde left a comment

SanchithHegde Aug 16, 2023 •

edited

anupj Sep 2, 2023

SanchithHegde Sep 2, 2023

anupj Sep 5, 2023

anupj Sep 6, 2023

lsampras Sep 7, 2023

SanchithHegde Aug 16, 2023

SanchithHegde commented Sep 9, 2023

anupj commented Sep 11, 2023

SanchithHegde commented Sep 11, 2023

fix(router): replace the occurrences of gen_range with a safer alternative #1908

fix(router): replace the occurrences of gen_range with a safer alternative #1908

Conversation

anupj commented Aug 9, 2023 • edited by SanchithHegde

Type of Change

Description

Additional Changes

Motivation and Context

How did you test it?

Checklist

Choose a reason for hiding this comment

anupj Aug 9, 2023 • edited

Choose a reason for hiding this comment

anupj Aug 10, 2023 • edited

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

anupj commented Aug 16, 2023

SanchithHegde left a comment

Choose a reason for hiding this comment

SanchithHegde Aug 16, 2023 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SanchithHegde commented Sep 9, 2023

anupj commented Sep 11, 2023

SanchithHegde commented Sep 11, 2023

fix(router): replace the occurrences of `gen_range` with a safer alternative #1908

fix(router): replace the occurrences of `gen_range` with a safer alternative #1908

anupj commented Aug 9, 2023 •

edited by SanchithHegde

anupj Aug 9, 2023 •

edited

anupj Aug 10, 2023 •

edited

SanchithHegde Aug 16, 2023 •

edited