Skip to content

Commit

Permalink
fix(subscriber): use saturating arithmetic for attribute updates (#234)
Browse files Browse the repository at this point in the history
Currently, an issue exists where a subtraction attribute update for a
resource or async op attribute may overflow, if a previous add update
was dropped due to event buffer capacity. This may result in the
aggregator task panicking. See
#180 (comment)
for details.

This branch changes all resource updates to use saturating arithmetic.
In practice, I think it's _much_ more likely for a subtract to overflow
due to missed adds than it is for an add to overflow due to missed subs,
but I made the addition saturating as well, just in case.

We should probably also increase the default event buffer capacity,
since it seems like lots of people are hitting problems with this...
  • Loading branch information
hawkw authored Dec 20, 2021
1 parent ef41507 commit fe82e17
Showing 1 changed file with 4 additions and 4 deletions.
8 changes: 4 additions & 4 deletions console-subscriber/src/aggregator/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1074,9 +1074,9 @@ fn update_attribute(attribute: &mut Attribute, update: &AttributeUpdate) {
(Some(DebugVal(v)), Some(DebugVal(upd))) => *v = upd,

(Some(U64Val(v)), Some(U64Val(upd))) => match update.op {
Some(AttributeUpdateOp::Add) => *v += upd,
Some(AttributeUpdateOp::Add) => *v = v.saturating_add(upd),

Some(AttributeUpdateOp::Sub) => *v -= upd,
Some(AttributeUpdateOp::Sub) => *v = v.saturating_sub(upd),

Some(AttributeUpdateOp::Override) => *v = upd,

Expand All @@ -1087,9 +1087,9 @@ fn update_attribute(attribute: &mut Attribute, update: &AttributeUpdate) {
},

(Some(I64Val(v)), Some(I64Val(upd))) => match update.op {
Some(AttributeUpdateOp::Add) => *v += upd,
Some(AttributeUpdateOp::Add) => *v = v.saturating_add(upd),

Some(AttributeUpdateOp::Sub) => *v -= upd,
Some(AttributeUpdateOp::Sub) => *v = v.saturating_sub(upd),

Some(AttributeUpdateOp::Override) => *v = upd,

Expand Down

0 comments on commit fe82e17

Please sign in to comment.