cc @alexbrainman @mattn @zx2c4

@gopherbot add OS-Windows

What should happen if a released callback is called?

Just panic, maybe? This should be the same situation as calling a released syscall/js.Func.

@hajimehoshi

Why do you need new syscall.ReleaseCallback function? What would you be able to do that you cannot do now?

I don't see any use for syscall.ReleaseCallback.

Thank you.

Alex

My understanding is that there is a limit for the number of the callbacks. If the number of calls of NewCallback reaches the limit, you can no longer create a new callback. Is that correct?

Or, at least, I'd want the documentation to make it explicit that a callback created by NewCallback is never released.

Actual example is waveOutOpen. This takes a callback each time when an audio stream is opened. If I didn't consider the limitation of NewCallback(CDecl), I would run into the issue of the limitation. So, I wanted a function to release the callback so that I could release it the audio stream ends, or I wanted to know the limitation so that I could avoid the limitation by reusing the same callback.

You're best off having one global callback for that, and using the dwInstance field in order to index into a map of actual callbacks.

(Alternatively, that function takes an event handle instead.)

I agree that NewCallback should document its limitations (I'll write up a CL for that soon if nobody beats me to it), but I think there are problems with something like ReleaseCallback. Like, if the callback is passed deep into a library, especially one that can trigger the callback asynchronously, the failure mode of releasing the callback without "de-registering" it is that either:

1. The slot is empty, and the callback attempts to jump to nil and crashes (that's fine), or
2. The slot is reused, and the callback calls into something totally unrelated.

(2) Is the case I'm worried about, because we can't really detect it and panic, at least not the way callbacks are implemented today. There might be some way to work around this, though. The limitation today stems from the fact that we identify the slot based on the return PC passed into callbackasm1, I'm not sure how else we can signal which callback we should use while still going down a single common path through the runtime to set up all the Go scheduler things that need to happen on the potentially external thread.

Actual example is waveOutOpen. This takes a callback each time when an audio stream is opened.

I would do what @zx2c4 suggested in #46184 (comment). I think one callback function should be enough in your scenario.

I also share @ianlancetaylor (#46184 (comment)) and @mknyszek (#46184 (comment)) concerns. There is no general way to inform external code to stop calling your callback function before you release it. This will cause memory corruption and crashes. It is not worth the trouble.

Alex

Change https://golang.org/cl/321133 mentions this issue: syscall: document NewCallback and NewCallbackCDecl limitations

There is no general way to inform external code to stop calling your callback function before you release it. This will cause memory corruption and crashes. It is not worth the trouble.

I think this is the same situation as syscall/js.Func's Release, and if we had ReleaseCallback, using this should be the user's responsibility.

Based on #46184 (comment), I'm now convinced that implementing ReleaseCallback would be difficult: calling a released callback cannot panic safely.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

Based on #46184 (comment), I'm now convinced that implementing ReleaseCallback would be difficult: calling a released callback cannot panic safely.

So it sounds like we should not add ReleaseCallback then?

I don't think we should not, but I'm not sure how feasible this would be. @mknyszek what do you think?

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

No change in consensus, so declined.
— rsc for the proposal review group