diff --git a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks.go b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks.go index beb5039b..9b48d69e 100644 --- a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks.go +++ b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks.go @@ -133,8 +133,9 @@ func decodeAssignerAsk(key string, data []byte) (assignerAsked, error) { // raiseAssignerAsk asks the operator the assigner's ask at key, once per router id: an ask answered already (from the view or // the phone) is not asked; an earlier asking still open is taken back first; an asking already made is only caught up with -// at the router, and one that ended is not asked again, for the router drops a second ask under an id it keeps. -func (t *Teller) raiseAssignerAsk(key string) { +// at the router, and one that ended is not asked again, for the router drops a second ask under an id it keeps. An asking +// still open is answered as catchUp, for the caller to catch up with once it let go of the teller's lock. +func (t *Teller) raiseAssignerAsk(key string) (catchUp string) { a, found, err := t.assignerAsk(key) if err != nil { t.logf("%s: the ask is not raised: %v", key, err) @@ -170,7 +171,7 @@ func (t *Teller) raiseAssignerAsk(key string) { } switch { case open(k): - t.catchUp(id) + catchUp = id case endedUnanswered(k) && !byHand: // The key stands under an asking that ended at the channel without an answer, heard now or at the teller's start // (issue 1966: the nats yes on novox and three backup asks stood so after #195, and nothing asked them again): the @@ -200,6 +201,7 @@ func (t *Teller) raiseAssignerAsk(key string) { } t.assignerAsked(id) t.logf("%s: the assigner's ask %s is asked of the operator (%s)", key, id, a.Ask.Headline) + return } // OnAssignerAskGone handles a key the assigner took away from its asks (a hold it lifted by itself): the open router ask of diff --git a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks_test.go b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks_test.go index 66375ff4..fe7ca68d 100644 --- a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks_test.go +++ b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/assignerasks_test.go @@ -10,6 +10,9 @@ import ( "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-teller/tellerevents" "strings" "testing" + "time" + + stdio "git.novox.be/novox/mesh-sdk/go" "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-assigner/asks" "git.novox.be/novox/mesh-delivery/modules/mesh-delivery-contracts/go/clients/meshdeliveryclient" @@ -292,3 +295,35 @@ func anAssignerAskWrittenAgainAfterBeingTakenAwayIsAskedAgainUnderANewAsking(t * } } } + +// An ask the teller caught up with inside a first view reads the router's record outside the teller's lock (principle 3, nothing +// stalls silently): while that read waits on the bus, a warrant the module's event pull hands on (Said) is handled, not held +// behind it. The order met: the ask is asked, the first view hands the key on again, and its catch-up's read does not answer. +func TestACatchUpWaitingOnTheRouterHoldsNoWarrant(t *testing.T) { + b := newBed(t) + assigner, _ := withView(b) + key, value, _ := holdAsk(t) + assigner[key] = value + b.t.OnAssignerAsk(key) + record := b.client.Record + entered, release := make(chan struct{}), make(chan struct{}) + b.client.Record = func(id string) (*stdio.StateEntry, error) { + close(entered) + <-release + return record(id) + } + caught := make(chan struct{}) + go func() { defer close(caught); b.t.OnAssignerAsk(key) }() // the first view hands the key on: the ask is caught up with + <-entered + said := make(chan struct{}) + go func() { defer close(said); b.t.Said(occ.Done, oc.Warrant{Ask: "other"}, nil) }() + select { + case <-said: + case <-time.After(5 * time.Second): + close(release) + <-caught + t.Fatal("a warrant waited on the teller's lock while a catch-up read the router's record") + } + close(release) + <-caught +} diff --git a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/teller.go b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/teller.go index 460b4dd4..355534a1 100644 --- a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/teller.go +++ b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/teller.go @@ -179,13 +179,20 @@ func (t *Teller) cancelOpen(key, id string) { } // catchUp reads the router's record of an ask the teller made and hands its warrant on, as the router's event would: -// a warrant that came while the teller was away. +// a warrant that came while the teller was away. It is called without the teller's lock and reads the record before taking +// it: the read may wait on the bus (the seat's holder, the router's state), and a warrant the module's event pull hands on +// (Said) waits on that lock, so nothing stalls behind the read (principle 3). An asking no longer open by then is left. func (t *Teller) catchUp(id string) { state, w, err := t.Asker.Where(id) + t.mu.Lock() + defer t.mu.Unlock() if err != nil { t.logf("the router's record of the ask %s could not be read: %v", id, err) return } + if k, _, err := t.keptOf(id); err != nil || !open(k) { + return + } if w == nil { if state == "" { // The router holds no record of an ask the teller holds open: it refused it (a refused ask is not kept) while @@ -285,7 +292,7 @@ func (t *Teller) Said(h occ.Handled, w oc.Warrant, err error) { t.said(h, w, err) } -// said is Said under the lock (catchUp holds it). +// said is Said under the lock (catchUp takes it once the router's record is read). func (t *Teller) said(h occ.Handled, w oc.Warrant, err error) { if h == occ.Refused && w.Outcome == occ.OutcomeRefused { // The client answers Refused with the router's words for an ask the router refused at take-in (design 47 G ยง570, @@ -517,15 +524,16 @@ func (t *Teller) endWait(id string) { // more when the router holds no record of it: the router keeps none of a refused ask (review of 38116fe0). Up to // occ.MostOpen are asked, the most the router can have room for. func (t *Teller) OnCurrent() { + // The router's records are read outside the teller's lock (as catchUp's): a read that waits on the bus holds no warrant. t.mu.Lock() - defer t.mu.Unlock() - ws, rec, err := t.waiting() + ws, _, err := t.waiting() + t.mu.Unlock() if err != nil { t.logf("%v", err) return } - changed := false - for i, x := range ws { + unheard := map[string]bool{} + for _, x := range ws { if !x.Asked { continue } @@ -535,6 +543,19 @@ func (t *Teller) OnCurrent() { continue } if state == "" && w == nil { + unheard[x.ID] = true + } + } + t.mu.Lock() + defer t.mu.Unlock() + ws, rec, err := t.waiting() + if err != nil { + t.logf("%v", err) + return + } + changed := false + for i, x := range ws { + if x.Asked && unheard[x.ID] { t.logf("%s: the ask %s, asked again, is not on the router's record: its refusal was not heard, and it waits again", x.Key, x.ID) ws[i].Asked, changed = false, true } diff --git a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/viewanswers.go b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/viewanswers.go index 16cff681..5673e82e 100644 --- a/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/viewanswers.go +++ b/modules/mesh-delivery-teller/cmd/mesh-delivery-teller/viewanswers.go @@ -33,14 +33,21 @@ func (t *Teller) OnViewAnswer(key string) { // OnAssignerAsk handles a key put in the assigner's bucket asks: a view answer that came before its ask fills it now, and an // ask not answered yet is raised on the operator channel (issue 1906; assignerasks.go). func (t *Teller) OnAssignerAsk(key string) { + if id := t.onAssignerAsk(key); id != "" { + t.catchUp(id) // outside the lock: it reads the router's record + } +} + +// onAssignerAsk is OnAssignerAsk under the teller's lock, answering the asking to catch up with. +func (t *Teller) onAssignerAsk(key string) string { t.mu.Lock() defer t.mu.Unlock() if _, err := viewstate.ViewAnswerKey(key); err != nil { t.logf("the assigner's key %s is left and never answered from the view: %v", key, err) - return + return "" } t.fillFromView(key) - t.raiseAssignerAsk(key) + return t.raiseAssignerAsk(key) } // fillFromView writes the answer to the assigner's ask at askKey from the view's answer, when both are there and readable, the