canonical / pebble

Take control of your internal daemons!
GNU General Public License v3.0
136 stars 51 forks source link

feat(planstate): run PlanChanged handlers lockless #395

Open flotter opened 3 months ago

flotter commented 3 months ago

Release the plan manager lock as soon as the new plan is created and plan manager plan pointer updated.

In the past all the plan consumer handlers were run while the plan lock was held. This is not necessary, since a new plan is always created following a plan change, and the new plan is never mutated by the plan manager or any consumers.

The following considerations were taken into account:

  1. Pebble API requests involving plan changes (i.e. AppendLayer, CombineLayer and SetServiceArgs) are synchronous and only complete (return) once all the PlanChanged callback handlers returned. This means that from a single API consumer point of view, once a change returns, the next API call made will be guaranteed to see Pebble in the updated state (single threaded consumer).

I've not considered a change in the design to make plan changes propagate asynchronously, as this would have changed the behaviour described above.

  1. The plan pointer sent to the PlanChanged consumers, following the release of the plan lock (the new design), must be a copy of the plan pointer already obtained before plan lock was released (otherwise we risk reading the plan pointer value during another change from the API).

The following changes are introduced:

flotter commented 3 months ago

@benhoyt I was thinking that before I move this PR to a final review state, whether you could take a quick peek to see if my approach feels acceptable for you ?