Thread safety

[marcplouhinec]

The goal of this pull request is to protect alicloud provider from the "thread-unsafety" of the underlying Go SDK. Because Terraform is creating resources in parallel (when possible), it randomly crashes with a panic error when a data race issue is occurring inside the Go SDK, because the later is NOT thread safe.

This pull request contains the following modifications:

• Create wrappers inside AliyunClient that forces each call to the Go SDK to be protected via a mutex (only one call can be done at a time). A wrapper is written in the following format:

  func (client *AliyunClient) RunSafelyWithEcsClient(do func(*ecs.Client) (interface{}, error)) (interface{}, error) {
      // Ensure that only one thread can call the Go SDK with the mutex "goSdkMutex"
      // Initialize the ecs.Client if not already initialized
      // Call do() and return its results
  }
  

  When some code needs to make a call to the Go SDK, it needs to use this wrapper. For example:

  // client is an instance of AliyunClient
  raw, err := client.RunSafelyWithEcsClient(func(ecsClient *ecs.Client) (interface{}, error) {
      return ecsClient.DescribeAvailableResource(args)
  })
  if err != nil {
      return "", nil, fmt.Errorf("Error DescribeAvailableResource: %#v", err)
  }
  resources, _ := raw.(*ecs.DescribeAvailableResourceResponse)
  

• Move AliyunClient into a new sub-package aliyunclient. The goal is to prevent the rest of the code (services, data sources, resources) to directly access Go SDK functions without the wrappers.
• Because functions located in service_*.go cannot be defined for AliyunClient anymore because they are now located in a different package than AliyunClient, a new struct has been created for each of them. For example calling the functionDescribeAvailableResources located in alicloud/service_alicloud_ecs.go become:

  ecsService := EcsService{client}
  zoneId, validZones, err := ecsService.DescribeAvailableResources(d, meta, InstanceTypeResource)
  

  This modification also makes the code easier to write and read, because related functions are now isolated into the same service structs.
• Modify each service, data source and resource in order to use the AliyunClient wrappers.
• Move constants and functions that were only used by the AliyunClient into the aliyunclient package.

All the acceptance tests have been executed in Frankfurt (eu-central-1). All of them successfully pass beside:

• TestAccAlicloudCSKubernetes_import
• TestAccAlicloudOtsInstance_import
• TestAccAlicloudOtsTable_importBasic
• TestAccAlicloudCSApplication_swarm
• TestAccAlicloudCSApplication_update
• TestAccAlicloudCSKubernetes_basic
• TestAccAlicloudCSSwarm_update
• TestAccAlicloudDBConnection_basic
• TestAccAlicloudOtsInstanceAttachment_Basic
• TestAccAlicloudOtsInstance_Basic
• TestAccAlicloudOtsInstance_Tags
• TestAccAlicloudOtsTable_Basic
• TestAccAlicloudVpnGateway_update
• TestAccAlicloudCSApplication_import

These tests were already failing for the same reasons before the changes included in this pull request.

The following tests have also been executed successfully in Beijing (cn-beijing):

• TestAccAlicloudSlb_spec
• TestAccAlicloudSlb_paybybandwidth
• TestAccAlicloudPvtzZoneRecordsDataSource_basic
• TestAccAlicloudPvtzZonesDataSource_basic
• TestAccAlicloudPvtzZoneAttachment_importBasic
• TestAccAlicloudPvtzZoneRecord_importBasic
• TestAccAlicloudPvtzZone_importBasic
• TestAccAlicloudPvtzZoneAttachment_Basic
• TestAccAlicloudPvtzZoneAttachment_multi
• TestAccAlicloudPvtzZoneAttachment_update
• TestAccAlicloudPvtzZoneRecord_Basic
• TestAccAlicloudPvtzZoneRecord_multi
• TestAccAlicloudPvtzZoneRecord_update
• TestAccAlicloudPvtzZone_Basic
• TestAccAlicloudPvtzZone_multi
• TestAccAlicloudPvtzZone_update
• TestAccAlicloudInstance_basic

[xiaozhu36]

I think package name client is not better because this package does not just contain client.

In addition, I think just use client.go is ok.

[xiaozhu36]

Please run make fmt before next submitting.

[marcplouhinec]

Strange, I do it again but it doesn't change any of my files. What happen if you do it on your computer?

[xiaozhu36]

I think line 629 can be removed and if it is not success, it will return an error.

[xiaozhu36]

I think args.Scheme = string(Https) will be better.

[marcplouhinec]

Unfortunately not possible because the Https constant is defined outside of the package.

[xiaozhu36]

I think using args.RegionId = client.RegionId will be better.

[xiaozhu36]

Why line 150 is csService.RunSafelyWithCsProjectClientByClusterName not client.RunSafelyWithCsProjectClientByClusterName

[xiaozhu36]

Please remove line 226-229.

[xiaozhu36]

I think line 14 just using *aliyunclient.AliyunClient is ok.

[marcplouhinec]

I disagree, the semantic changes when you remove "client": in the current implementation a service uses the client, but in what you propose the service extends the client.

Although it is slightly more verbose to write "myService.client.SomeFunction" instead of "myService.SomeFunction", it is easier to read because the reader can immediately understand that the function come from the client, not the service.

It's like the Composition over inheritance principle.

[xiaozhu36]

I think line 90 uses args.RegionId =client.RegionId will be better.

[xiaozhu36]

This PR aims to solve GO SDK thread-unsafe problem. I think the prefix RunSafelyWith is redundant and it can be removed to simplify the client definition. If you want to stress thread-safe, adding some annotations is good choice.

[marcplouhinec]

I propose to still keep "With" like "WithSomeClient", if not it would look like a weird getter.