Skip to content

Commit

Permalink
Fix retry session info request
Browse files Browse the repository at this point in the history
When fetching session info, the client would not retry in cases where
the request was successfully sent but no response was received.

This change only impacts BLE, since HTTP requests are synchronous.
  • Loading branch information
Seth Terashima committed Jan 29, 2025
1 parent ebad42a commit 60cf1f9
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 18 deletions.
47 changes: 30 additions & 17 deletions internal/dispatcher/dispatcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,27 +87,40 @@ func (d *Dispatcher) StartSession(ctx context.Context, domain universal.Domain)
return err
}
for {
recv, err := d.RequestSessionInfo(ctx, domain)
if err != nil {
if retry, err := d.tryStartSession(ctx, s, domain); !retry {
return err
}
defer recv.Close()
select {
case reply := <-recv.Recv():
if err = protocol.GetError(reply); err != nil {
return err
}
case <-ctx.Done():
return ctx.Err()
case <-s.readySignal:
return nil
}
select {
case <-time.After(d.conn.RetryInterval()):
case <-ctx.Done():
return ctx.Err()
}
}

func (d *Dispatcher) tryStartSession(ctx context.Context, s *session, domain universal.Domain) (retry bool, err error) {
recv, err := d.RequestSessionInfo(ctx, domain)
if err != nil {
return false, err
}
defer recv.Close()
// Request sent
select {
case <-ctx.Done():
return false, ctx.Err()
case <-s.readySignal:
return false, nil
case <-time.After(d.RetryInterval()):
return true, nil
case reply := <-recv.Recv():
if err = protocol.GetError(reply); err != nil {
return false, err
}
}
// Reply received. Normally, the dispatcher will clear readySignal after processing the reply.
select {
case <-s.readySignal:
return false, nil
case <-ctx.Done():
return false, ctx.Err()
case <-time.After(d.RetryInterval()):
return true, nil
}
}

// StartSessions starts sessions with the provided vehicle domains (or all supported domains, if
Expand Down
47 changes: 46 additions & 1 deletion internal/dispatcher/dispatcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ func TestWaitForAllSessions(t *testing.T) {

// Configure the Connector to only respond to the first of two handshakes
conn.EnqueueSendError(nil)
conn.EnqueueSendError(errDropMessage)
conn.dropReplies = true

key, err := authentication.NewECDHPrivateKey(rand.Reader)
if err != nil {
Expand Down Expand Up @@ -928,6 +928,51 @@ func TestNoValidHandshakeResponse(t *testing.T) {
}
}

func TestRetryNonresponsive(t *testing.T) {
// Verifies that the client tries to resend session info requests to non-responsive domains
conn := newDummyConnector(t)
defer conn.Close()

key, err := authentication.NewECDHPrivateKey(rand.Reader)
if err != nil {
t.Fatal(err)
}

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

dispatcher, err := New(conn, key)
if err != nil {
t.Fatal(err)
}
if err := dispatcher.Start(ctx); err != nil {
t.Fatal(err)
}
defer dispatcher.Stop()

const maxCallbacks = 5
callbackCount := 0

conn.callback = func(_ *dummyConnector, message *universal.RoutableMessage) ([]byte, bool) {

Check failure on line 956 in internal/dispatcher/dispatcher_test.go

View workflow job for this annotation

GitHub Actions / build

unused-parameter: parameter 'message' seems to be unused, consider removing or renaming it as _ (revive)
t.Log("Received callback")
callbackCount++ // caller holds conn.lock
if callbackCount >= maxCallbacks {
cancel()
}
return nil, false
}

if err := dispatcher.StartSession(ctx, testDomain); !errors.Is(err, context.Canceled) {
t.Errorf("Expected key not paired but got %s", err)
}

conn.lock.Lock()
defer conn.lock.Unlock()
if callbackCount < maxCallbacks {
t.Errorf("Expected %d callbacks, got %d", maxCallbacks, callbackCount)
}
}

func TestCache(t *testing.T) {
conn := newDummyConnector(t)
key, err := authentication.NewECDHPrivateKey(rand.Reader)
Expand Down

0 comments on commit 60cf1f9

Please sign in to comment.