registry: fix panic 'attempt to add with overflow' #79
Conversation
|
Actually it's not checked in release mode. |
| @@ -58,7 +58,7 @@ impl RegistryCore { | |||
| // the collector_id. | |||
| if desc_id_set.insert(desc.id) { | |||
| // The set did not have this value present, true is returned. | |||
| collector_id += desc.id; | |||
| collector_id ^= desc.id; | |||
There was a problem hiding this comment.
can we use wrapping_add instead?
There was a problem hiding this comment.
Yes, we can, but why not xor?
There was a problem hiding this comment.
Why does collector_id need to plus desc.id in the past? Why not just plus 1?
There was a problem hiding this comment.
Yes, we can, but why not xor?
Keep the same with Go
Why does collector_id need to plus desc.id in the past? Why not just plus 1?
Port from Go directly.
There was a problem hiding this comment.
It is ported from client_golang.
A collector may collect multiple metrics, thus a collector's id would be the sum of metrics's id. Anyway, our goal here is computing a collector's id based on it's metrics.
|
LGTM |
| @@ -58,7 +59,7 @@ impl RegistryCore { | |||
| // the collector_id. | |||
| if desc_id_set.insert(desc.id) { | |||
| // The set did not have this value present, true is returned. | |||
| collector_id += desc.id; | |||
| collector_id = (Wrapping(collector_id) + Wrapping(desc.id)).0; | |||
There was a problem hiding this comment.
can we use https://doc.rust-lang.org/beta/std/primitive.i64.html#method.wrapping_add directly?
|
LGTM |
When we register a collector that returns multiple descriptors, registry will panic and leaves a message:
Integer overflow is considered unsafe in rust. Rust checks it at both compile time and runtime.
Although there are several ways to workaround, like scoped-attributes and wrapping,
I would recommend xor. I think xor is enough in our use case.Now, wrapping add.
PTAL @siddontang @disksing