DavidBM / rsmq-async-rs

RSMQ port to async rust. RSMQ is a simple redis queue system that works in any redis v2.6+
MIT License
43 stars 8 forks source link

Max duration of delay when sending to queue #17

Closed emmiegit closed 7 months ago

emmiegit commented 7 months ago

Hello, I've been using rsmq-async to implement a job queue, and for one of my use cases I want to run a job after one day. However it seems this surpasses the limit delay values can have, and I'm not sure I understand why.

The error reported is that 86400000 > 9999999, which was confusing to me since I was entering 86400, which is well below 9999999. Looking at send_message in the Rust docs makes no mention of a maximum value for delay, other than the implicit maximum value for a u64.

When I opened the code and began reading through, I see that within send_message the delay value is first multipled by 1000, and then it is compared against 9_999_999.

Given this, a few questions:

Thank you in advance.

DavidBM commented 7 months ago

Hi! You are right that it doesn't look right.

After checking the code a bit I changed it to be seconds rather than millis. I published a version 7 of the crate and added a test for the delays. It should work as expected now.

The numer_in_range function comes from the internal check the js counterpart of this library did. Do it is more for compatibility with it. In the future I might try to extend that.

In any case, now the number is not multiplied, so you have a max delay of 115 days.

emmiegit commented 7 months ago

Thanks for the response! I've tested the new version, and it looks like it works fine with 1 day-jobs. I think the only remaining item in this issue is my suggestion to document the maximum values in the crate docs, other than that I believe the core question raised in this issue is resolved.

DavidBM commented 7 months ago

After some thinking I changed the variables to use the std::time::Duration and be stored in milliseconds rather than seconds in Redis. Please, test that it works with the 1 day jobs you are planning to use.

Also, please, keep in mind that this is a breaking change (published under version 8, following SemVer), so make sure that there are no messages when you deploy the new version. You can use another queue prefix if you want to keep running both version during some time.

emmiegit commented 7 months ago

Thank you for the notice! I appreciate the Duration change, it makes it easier for API users to be certain about the duration they're requesting since it is not unitless.