Make sure only one package syncing operation happens at a time

[kpratyus]

This is the revised PR for issue #84 . As proposed I am in the process to write a test case for testing the changes (It will take some time as I am new to the codebase).

[codecov]

Codecov Report

Merging #120 (3e0abca) into main (3d4cbdc) will increase coverage by 0.36%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #120      +/-   ##
==========================================
+ Coverage   74.89%   75.25%   +0.36%     
==========================================
  Files          22       22              
  Lines        1629     1637       +8     
==========================================
+ Hits         1220     1232      +12     
+ Misses        307      304       -3     
+ Partials      102      101       -1     

Impacted Files	Coverage Δ
client/internal/clientcommon.go	79.50% <ø> (ø)
client/httpclient.go	94.64% <100.00%> (+0.09%)	⬆️
client/internal/httpsender.go	72.26% <100.00%> (ø)
client/internal/packagessyncer.go	60.00% <100.00%> (+0.74%)	⬆️
client/internal/receivedprocessor.go	69.71% <100.00%> (+0.43%)	⬆️
client/internal/wsreceiver.go	91.42% <100.00%> (ø)
client/wsclient.go	87.11% <100.00%> (+2.54%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d4cbdc...3e0abca. Read the comment docs.

[kpratyus]

Hi @tigrannajaryan , one case which could lead to race is when a package having same name, version and hash as previous package is tried to be Installed. Ideally the packagesyncer should ignore the request but due to race condition it could try to install it again. Do you have any other cases on top of your mind?

[tigrannajaryan]

Hi @tigrannajaryan , one case which could lead to race is when a package having same name, version and hash as previous package is tried to be Installed. Ideally the packagesyncer should ignore the request but due to race condition it could try to install it again. Do you have any other cases on top of your mind?

I think the most common case we want to test for is this:

1. Agent receives a ServerToAgent message with PackagesAvailable field set.
2. Agent creates a corresponding PackagesSyncer and begins downloading and syncing the packages. This happen in packagesSyncer.doSync on a different goroutine.
3. Agent receives another ServerToAgent message with PackagesAvailable field set. The value of PackagesAvailable may be different than the first time (i.e. something has changed on the Server side, while step 2 was executing.
4. Agent creates another PackagesSyncer and calls Sync(). This syncing should wait until the previous one in Step 2 is complete.

[tigrannajaryan]

Looks good so far. Let's add a test and should be good to merge

[tigrannajaryan]

Nit: we use "Mutex" prefix elsewhere in this codebase.

Suggested change

	PackageSyncComplete sync.Mutex
	PackageSyncMutex sync.Mutex

[kpratyus]

Facing the following race condition. Trying to debug it. Any help would be appreciated.

Read at 0x00c000380da8 by goroutine 24:
  runtime.slicecopy()
      /usr/lib/golang/src/runtime/slice.go:284 +0x0
  google.golang.org/protobuf/internal/impl.mergeBytesNoZero()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/merge.go:167 +0xc6
  google.golang.org/protobuf/internal/impl.(*MessageInfo).mergePointer()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/merge.go:52 +0x2b5
  google.golang.org/protobuf/internal/impl.(*MessageInfo).merge()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/merge.go:32 +0x9a
  google.golang.org/protobuf/internal/impl.(*MessageInfo).merge-fm()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/merge.go:23 +0x84
  google.golang.org/protobuf/proto.mergeOptions.mergeMessage()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/proto/merge.go:73 +0xc8
  google.golang.org/protobuf/proto.Clone()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/proto/merge.go:58 +0x84
  github.com/open-telemetry/opamp-go/client/internal.(*ClientSyncedState).SetPackageStatuses()
      /opamp-go/client/internal/clientstate.go:121 +0x67
  github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).reportStatuses()
      /opamp-go/client/internal/packagessyncer.go:336 +0x1c6
  github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).syncPackage()
      /opamp-go/client/internal/packagessyncer.go:223 +0xa5e
  github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).doSync()
      /opamp-go/client/internal/packagessyncer.go:127 +0x4e5
  github.com/open-telemetry/opamp-go/client/internal.(*packagesSyncer).Sync·dwrap·11()
      /opamp-go/client/internal/packagessyncer.go:65 +0x64

Previous write at 0x00c000380da8 by goroutine 72:
  runtime.slicecopy()
      /usr/lib/golang/src/runtime/slice.go:284 +0x0
  google.golang.org/protobuf/internal/impl.consumeBytesNoZero()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/codec_gen.go:5410 +0xe5
  google.golang.org/protobuf/internal/impl.(*MessageInfo).unmarshalPointer()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/decode.go:139 +0x79d
  google.golang.org/protobuf/internal/impl.consumeMessageInfo()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/codec_field.go:252 +0x25c
  google.golang.org/protobuf/internal/impl.(*MessageInfo).unmarshalPointer()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/decode.go:139 +0x79d
  google.golang.org/protobuf/internal/impl.(*MessageInfo).unmarshal()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/decode.go:62 +0x104
  google.golang.org/protobuf/internal/impl.(*MessageInfo).unmarshal-fm()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/internal/impl/decode.go:55 +0x104
  google.golang.org/protobuf/proto.UnmarshalOptions.unmarshal()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/proto/decode.go:93 +0x2bd
  google.golang.org/protobuf/proto.Unmarshal()
      /root/go/pkg/mod/google.golang.org/protobuf@v1.27.1/proto/decode.go:50 +0xa4
  github.com/open-telemetry/opamp-go/client/internal.(*wsReceiver).receiveMessage()
      /opamp-go/client/internal/wsreceiver.go:73 +0x67
  github.com/open-telemetry/opamp-go/client/internal.(*wsReceiver).ReceiverLoop()
      /opamp-go/client/internal/wsreceiver.go:55 +0xf7
  github.com/open-telemetry/opamp-go/client.(*wsClient).runOneCycle()
      /opamp-go/client/wsclient.go:242 +0x5cf
  github.com/open-telemetry/opamp-go/client.(*wsClient).runUntilStopped()
      /opamp-go/client/wsclient.go:264 +0x44
  github.com/open-telemetry/opamp-go/client.(*wsClient).runUntilStopped-fm()
      /opamp-go/client/wsclient.go:257 +0x4d
  github.com/open-telemetry/opamp-go/client/internal.(*ClientCommon).StartConnectAndRun.func1()
      /opamp-go/client/internal/clientcommon.go:199 +0xb2

[srikanthccv]

@kpratyus are you still interested in continuing this work?

[tigrannajaryan]

Closing since there hasn't been updates to the PR for a while. Please reopen if still active.