[resources/armresources] Fake servers can send messages to closed channel

[JGiola]

Bug Report

In using the fake server for sdk/resources/armresources if during the test the context has been cancelled for whatever reason a panic for sending a message to an already closed channel can be triggered.

The main culprit is this function:

func (s *ServerTransport) dispatchToMethodFake(req *http.Request, method string) (*http.Response, error) {
	resultChan := make(chan result)
	defer close(resultChan)

	go func() {
		var intercepted bool
		var res result
		if serverTransportInterceptor != nil {
			res.resp, res.err, intercepted = serverTransportInterceptor.Do(req)
		}
		if !intercepted {
			switch method {
			case "Client.CheckExistence":
				res.resp, res.err = s.dispatchCheckExistence(req)
			case "Client.CheckExistenceByID":
				res.resp, res.err = s.dispatchCheckExistenceByID(req)
			case "Client.BeginCreateOrUpdate":
				res.resp, res.err = s.dispatchBeginCreateOrUpdate(req)
			case "Client.BeginCreateOrUpdateByID":
				res.resp, res.err = s.dispatchBeginCreateOrUpdateByID(req)
			case "Client.BeginDelete":
				res.resp, res.err = s.dispatchBeginDelete(req)
			case "Client.BeginDeleteByID":
				res.resp, res.err = s.dispatchBeginDeleteByID(req)
			case "Client.Get":
				res.resp, res.err = s.dispatchGet(req)
			case "Client.GetByID":
				res.resp, res.err = s.dispatchGetByID(req)
			case "Client.NewListPager":
				res.resp, res.err = s.dispatchNewListPager(req)
			case "Client.NewListByResourceGroupPager":
				res.resp, res.err = s.dispatchNewListByResourceGroupPager(req)
			case "Client.BeginMoveResources":
				res.resp, res.err = s.dispatchBeginMoveResources(req)
			case "Client.BeginUpdate":
				res.resp, res.err = s.dispatchBeginUpdate(req)
			case "Client.BeginUpdateByID":
				res.resp, res.err = s.dispatchBeginUpdateByID(req)
			case "Client.BeginValidateMoveResources":
				res.resp, res.err = s.dispatchBeginValidateMoveResources(req)
			default:
				res.err = fmt.Errorf("unhandled API %s", method)
			}

		}
		select {
		case resultChan <- res:
		case <-req.Context().Done():
		}
	}()

	select {
	case <-req.Context().Done():
		return nil, req.Context().Err()
	case res := <-resultChan:
		return res.resp, res.err
	}
}

As we can see a gofunction is created with a final select will send to resultChan the response from the mocked function or listen if the context done channel has been closed.

In the main function a similar select will send the respond listening to the context or result chan. once one of these conditions will be met the function will return and the defer function will be called closing resultChan.

The bug can be triggered if the function is called with an already cancelled context (for whatever reason of concurrency during codes, or for a timeout). In this case if the main function will end before the gofunction the final select will send the context error immediately closing the channel but then the gofunction will attempt to send a message to it because the case resultChan <- res is coming before the case <- req.Context.Done().

This same behaviour can be seen in every fake server implementation of the module.

A possible solution can be to switch the select cases inside the gofunction, so if at the end the context is cancelled we don't send the message, or to move the defer inside the gofunction for avoiding sending a message to a closed channel.

A simple reproduction test is this:

func TestSendCloseChannel(t *testing.T) {
	ctx, cancel := context.WithCancel(t.Context())
	cancel()

	client, err := armresources.NewClient("00000000-0000-0000-0000-000000000000", &fakeazcore.TokenCredential{}, &arm.ClientOptions{
		ClientOptions: policy.ClientOptions{
			Transport: fakearmresources.NewServerTransport(&fakearmresources.Server{
				GetByID: func(_ context.Context, resourceID, apiVersion string, _ *armresources.ClientGetByIDOptions) (responder fakeazcore.Responder[armresources.ClientGetByIDResponse], errResponder fakeazcore.ErrorResponder) {
					return responder, errResponder
				},
			}),
		},
	})
	require.NoError(t, err)

	_, err = client.GetByID(ctx, "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/myRG", "2021-04-01", nil)
	if !errors.Is(err, context.Canceled) {
		t.Fatalf("unexpected error: %v", err)
	}
}

The problem can be triggered running the test with go test -count 500 or better yet with go test -race -count 500. If you ran it once it can either fail or pass based on how the routines will be called.

The version of the sdks I'm using are:

• github.com/Azure/azure-sdk-for-go/sdk/azcore v1.21.0
• github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources/v3 v3.0.1