Release AsyncOperationCompletedHandler

[spreatty]

I was working on a home-automation project that involves Switchbots, actually 2 of them. I do quite many calls to them and I faced a crash after about 20 minutes of work. The stack trace was so big that I couldn't see the error, but what I could see is that there was over 500 goroutines spawned by NewAsyncOperationCompletedHandler. And then I noticed that a channel for the handler is never released.

This PR simply releases channel for AsyncOperationCompletedHandler.

[aykevl]

LGTM (though I don't really know much about the Windows variant)

Just to be clear: you tested this and confirmed this patch fixes the bug?

[deadprogram]

cc @jagobagascon

[jagobagascon]

This looks like a memory leak, we were calling NewAsyncOperationCompletedHandler and never releasing the created object.

Regarding the hundreds of goroutines:

A few weeks ago we introduced a change to remove the need for CGO in WinRT-Go (saltosystems/winrt-go#60). This was developed more than a year ago, but wasn't merged because of a bug in the Go runtime that makes apps panic with a deadlock error (golang/go#55015).

To workaround that bug we spawn goroutines during the lifecycle of delegates.

Edit:
Releasing a delegate automatically closes the channel used for that instance.

[jagobagascon]

I would probably release the handler using defer, this way it's clear what we are releasing:

// Wait until the async operation completes.
waitChan := make(chan struct{})
handler := foundation.NewAsyncOperationCompletedHandler(ole.NewGUID(iid), func(instance *foundation.AsyncOperationCompletedHandler, asyncInfo *foundation.IAsyncOperation, asyncStatus foundation.AsyncStatus) {
	status = asyncStatus
	close(waitChan)
})
defer handler.Release()
	
asyncOperation.SetCompleted(handler)

[spreatty]

Thanks. This looks even better

[spreatty]

Tested it more and it turned out, this is not the only problem. This PR patches memory and goroutine leak, but there is a bigger issue: "too many callbacks". I dig a bit into syscall.NewCallback and found out that we can only create 2000 (as per go 1.21.4) callbacks, which is 500 async ops.

I will fork saltosystems/winrt-go and reuse callback for the handlers so that we spawn no more than 8 callbacks (4 for AsyncOperationCompletedHandler and 4 for TypedEventHandler). This will be a temporary solution and should not be merged, i.e. I won't create a PR, but I will share a reference to the branch. The reason is IUnknown::AddRef and IUnknown::Release must not take arguments and it's impossible to tell which handler a particular call is related to. The solution is to make refs app-wide. That does not create much overhead, app-wide refs reach 0 after every group of operations like connect->discover->write. However, once you enable notifications you create a memory leak and refs will never reach zero. It's fine for my app, although it will lack some features, like indicating devices battery level.

Also, how was it before removing CGO? Can we create any number of callbacks with CGO? What was the reason for getting rid of CGO?

[jagobagascon]

@spreatty AddRef and Release must not take arguments, but they do receive the pointer to the underlying object.

I tested it and can see how each instance is released, each line in this log is a call to Release:

1230736 : remaining references 2
1230736 : remaining references 1
1230736 : remaining references 0
1230736 RELEASED
1459856 : remaining references 2
1459856 : remaining references 1
1459856 : remaining references 0
1459856 RELEASED

I made a quick change to WinRT so that all delegates share the same four callbacks, and it seems to be working fine in my tests.

@spreatty could you please test this branch I created? https://github.com/saltosystems/winrt-go/tree/feature/reuse-syscall-callbacks-to-avoid-reaching-the-Golang-limit

[spreatty]

Oddly, I got errors after adding pointers into arguments. Perhaps I made a mistake somewhere. I will test your branch, thanks

[spreatty]

@jagobagascon I can start my app with that branch and I think I won't see callbacks issue. However, I'd like to ask you making delegate.RegisterCallbacks method concurrently safe. Sometimes when connecting 2 devices, the app initiates service discovering simultaneously for both. Although there are shared callbacks under the hood, each device looks like a fully autonomous entity and it's confusing that I need to sync operations on different devices

[jagobagascon]

Happy to hear that! Sure, the change was just to quickly test if it worked, I'll review it and make it thread safe.

Anyway, that has nothing to do with this PR so I created an issue for this particular problem: saltosystems/winrt-go#82.

[spreatty]

@jagobagascon What about this PR? Should merge it or just leave it?

[jagobagascon]

This PR fixes a memory leak, so yes, it should be merged.

[spreatty]

@jagobagascon Who must do it? Should we tag someone?

[deadprogram]

Now squash/merging. Thank you for the fix @spreatty and to @jagobagascon and @aykevl for review.

[jagobagascon]

saltosystems/winrt-go#82 has been merged, it fixes the callbacks issue @spreatty was having.

[deadprogram]

We need to update this repo once there is a new release of winrt-go is that correct @jagobagascon ?

[jagobagascon]

Yes, but there's no releases for WinRT-Go, the main branch is considered stable, so just point to the new commit.