Go 1.6beta1 - Changes to CGO pointer passing break portaudio

[undefinedopcode]

Hi,

I tried compiling portaudio against the new golang 1.6 beta 1 release and it breaks port audio bindings. As a part of 1.6 they tighten the rules regarding Go pointers being passed into C memory.

This means that while programs compile, at run-time they abort. See below for the noise.go example from the distribution.

This example worked fine on 1.5.x and lower.

The related changes are described in golang/go#12416.

$ ./noise.exe
panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 1 [running]:
github.com/gordonklaus/portaudio._cgoCheckPointer1(0x4b8d00, 0x1298e230, 0x0, 0x0, 0x0, 0x4b1dc0)
        ??:0 +0x41
github.com/gordonklaus/portaudio.OpenStream(0x0, 0x0, 0x0, 0x0, 0x12988300, 0x2, 0x989680, 0x0, 0x0, 0x40e58880, ...)
        C:/Users/april/Projects/super-eight/GoPath/src/github.com/gordonklaus/portaudio/portaudio.go:504 +0x2f5
main.main()
        c:/Users/april/Projects/super-eight/GoPath/src/github.com/gordonklaus/portaudio/examples/noise.go:18 +0x104

[sqweek]

Ian Taylor's design document about the changes suggests a way forward:

A particular unsafe area is C code that wants to hold on to Go func and pointer values for future callbacks from C to Go. This works today but is not permitted by the invariant. It is hard to detect. One safe approach is: Go code that wants to preserve funcs/pointers stores them into a map indexed by an int. Go code calls the C code, passing the int, which the C code may store freely. When the C code wants to call into Go, it passes the int to a Go function that looks in the map and makes the call. An explicit call is required to release the value from the map if it is no longer needed, but that was already true before.

I'm not sure if the blocking API also needs tweaking, but if so allocating the buffers in C via malloc rather than in Go itself should be simple enough?

[Orion90]

Hi!

I've made a fix for this in my fork of this repository, but can't find any contribution guidelines. Should I just submit a PR or is there another preferred way to contribute?

[sqweek]

Hi Orion90! I can't speak for Gordon regarding contribution guidelines but I've finally gotten around to looking at your patch. Allocating memory for the Stream structure via malloc seems like a reasonable strategy, but:

1. It then needs to be freed at some point to avoid leaking memory,
2. It seems like you are allocating the memory much too late - the memory pointed to by bs is allocated but not initialised (which gets passed to Pa_OpenStream). I guess you must be using the blocking API as otherwise this would surely segfault when the callback is invoked?

[Orion90]

Hey! Thanks for the feedback!

1. Yeah, I realised that right after commenting on this issue.
   But haven't time to investigate the codebase to figure out where I should free it. I just tried to make it work for a small project I'm doing.
2. Thanks, yeah I've only tried it with code based on the record example. Will dig into it a bit more and update my repo accordingly :)

I'll post a comment here when I've updated it, if someone doesn't beat me to it.

[sqweek]

Stream.Close would be the place to free it. But after revisiting the previously linked design document I'm not sure the malloc approach is going to help - one of the rules is:

Go code may not store a Go pointer in C memory.

Still, it's good to know the blocking API works with your change. It suggests that the address of the callback function is the only hurdle.

[helinwang]

Hi, now go1.6 is released, any plan to fix it? I can help to code review or if anyone have working patch I can help clean it up.

[helinwang]

My pull request fixes this issue #10, I would love some feedback.

[jaredfolkins]

Nice start @helinwang! It does appear that you are adhering to Ian Taylor's design document which @sqweek referenced.

I refactored the code to make it a bit more composable. I also found a case in Start() where we could add a pointer to the map but an error may never release it.

I debated on what to call the functions and am open to suggestions. I didn't figure Release() or Free() would work because we aren't truly operating on memory, we are simply no longer tracking a pointer so I implemented Track() and Untrack().

I am sure there are further issues with my code too. More eyeballs on it would be wonderful.

[jaredfolkins]

Man, my gofmt vim plugin is making that diff bleed way more red than needed. I really didn't change that much.

[jaredfolkins]

I'm going to fix whatever happened to the formatting so the diff is better represented. That is just too much fuglyness to ask people to look through.

[jaredfolkins]

Alright that is a bit better.

jaredfolkins@90822d4

[jaredfolkins]

Tested @nf's Sigourney using my branch. It appears to work. Again, I'm almost certain I'm leaking something but I'm too tired. Stopping to eat.

[sqweek]

Calling mutex.Lock within streamCallback is not good - look at any audio processing mailing list and you'll have no trouble finding experienced coders saying "never ever do anything in that has the potential to block in the audio callback".

The good news it that the lock is not necessary at this stage. The callback will never be invoked before Pa_OpenStream is called, so by the time we get there we know that idToStream has been filled out. The only reason it might be nil is if Stream.Close has just been called; IMO there's no need to panic in that case, and it can probably be avoided by untracking the callback after calling Pa_CloseStream. I think the only thing the mutex is actually required for is to ensure that each stream gets a unique id (which could also be achieved via sync/atomic but it doesn't make much difference).

Also the PR touches too many places in the code. The only functions that need to care about the mapping are Stream.Open and Stream.Close - they mark the go/c callback boundary. The addition of NewStream is fair enough, otherwise you don't know which id to untrack in Close. I think it should be renamed to newStream though; it's not part of the public API.

[helinwang]

@sqweek thanks for the comment. I have one question about locking - bear with me if I am wrong, I am not totally confident about understanding of synchronization.

I thought lock is necessary in stream callback because map maybe modified from different go routine. E.g, the open/close go routine. If only modified from open go routine, it kind of make sense: open surely will happen before callback, so lock as a memory barrier will make sure modification is propagated to different go routines. But it's not the case for close.

Maybe change to RW lock will solve the potential latency issue? --- btw, I think even lock (not RWLock) is pretty fast.

[helinwang]

@jaredfolkins thanks for the comments, yes you are right, many places error case prevent *Stream from removed from the map. One idea, maybe use unsafe.SetFinalizer to do it?

[sqweek]

If you call Pa_CloseStream before removing the pointer from the map then it shouldn't matter in the close case either. If we want to be really cautious a better compromise would be to spawn a go routine in close that sleeps for a few seconds before clearing the map entry. Doesn't matter if it hangs around a bit longer, we just need to ensure it is there while the callback is active.

I agree mutex is fast on average, but it's the worst case we need to worry about because that is what will cause glitches in your audio stream. If you try to acquire the lock but someone else has it and the go routine running the callback goes to sleep, when does it get rescheduled? Who guarantees that it gets rescheduled before the deadline?

Maybe it does usually wake up in time, I don't know for sure. But I don't feel introducing this kind of uncertainty is acceptable behaviour for an audio library.

Also you can start and stop a steam many times in between open and close, so errors on those code paths shouldn't affect the map.

-----Original Message-----
From: "helinwang" notifications@github.com
Sent: ‎26/‎02/‎2016 23:34
To: "gordonklaus/portaudio" portaudio@noreply.github.com
Cc: "sqweek" sqweek@gmail.com
Subject: Re: [portaudio] Go 1.6beta1 - Changes to CGO pointer passing breakportaudio (#7)

@sqweek thanks for the comment. I have one question about locking - bear with me if I am wrong, I am not totally confident about understanding of synchronization.
I thought lock is necessary in stream callback because map maybe modified from different go routine. E.g, the open/close go routine. If only modified from open go routine, it kind of make sense: open surely will happen before callback, so lock as a memory barrier will make sure modification is propagated to different go routines. But it's not the case for close.
Maybe change to RW lock will solve the potential latency issue? --- btw, I think even lock (not RWLock) is pretty fast.
—
Reply to this email directly or view it on GitHub.

[jaredfolkins]

@sqweek @helinwang nice job talking this out. I'll add some more thoughts and research.

What does PortAudio do?

PortAudio has a example program that actually uses a ringbuffer to help with performance on a blocking operation.

https://subversion.assembla.com/svn/portaudio/portaudio/trunk/pablio/pablio.c

What is the rest of the Golang/Cgo community doing?

I used some Search-Fu to find examples of what other community members are doing.

The results are here which allowed me to track down this commit here. What is hilarious is that this commit is very similar to my initial commit for PortAudio which I wrote without even searching around.

Here is another one, using channels, which I think is the wrong approach.

One idea, maybe use unsafe.SetFinalizer to do it?

I found this post/debate about a proposal for depricating SetFinalizer(). The proposal was withdrawn but it does bring up some interesting stuff. What is everyone's thoughts about it?

https://groups.google.com/forum/#!topic/golang-dev/DMiUkpS1uyQ

Maybe change to RW lock will solve the potential latency issue? --- btw, I think even lock (not RWLock) is pretty fast.

Yes, we should most certainly do this for read operations.

https://golang.org/pkg/sync/#RWMutex.RLock

The good news it that the lock is not necessary at this stage. The callback will never be invoked before Pa_OpenStream is called, so by the time we get there we know that idToStream has been filled out.

Any operation on a map with atomicity requires locking. Reading, Writing, Updating, Deleting. All must lock.

https://news.ycombinator.com/item?id=10986069

Also you can start and stop a stream many times in between open and close, so errors on those code paths shouldn't affect the map.

I think it would depend on the type of error no?

---

My vote is that we don't fear locking, it is insanely fast. You'll probably run into a bottle neck with millions of operations per second but I don't think we have to worry about that for now. If it does become an issue, we could just use some fancy ring hash to make it performant.

Keep posting your thoughts. At least we are all taking the time to figure out the best thing for the library.

-jf

[helinwang]

Thanks for the detailed reply @jaredfolkins Yes you are right, the Unlock does not propagate information to all other go routine. It's only guaranteed that next mu.Lock() will happen before mu.Unlock(). So I think lock is necessary this case in this call back. Reference https://golang.org/ref/mem#tmp_8

I my experience SetFinalizer is a good use case for cgo where you allocate a c memory and need to free it when the owner go object goes away (don't know if there is any better solution than this)

[jaredfolkins]

Another thread. Another similar solution. We are on the right track.

evmar/gocairo#3
evmar/gocairo@6a65339

[sqweek]

PortAudio has a example program that actually uses a ringbuffer to help with performance on a blocking operation.

Yes, the point of the ringbuffer is to avoid blocking in the audio callback. I'm not making this issue up, it took me two minutes to find these snippets:

• http://stackoverflow.com/questions/23194197/using-a-mutex-in-an-audio-callback
• Locking and memory allocation in JACK callback are naughty bgribble/mfp#42
• http://www.rossbencina.com/code/real-time-audio-programming-101-time-waits-for-nothing (Ross is one of the authors behind portaudio)
• http://portaudio.com/docs/v19-doxydocs/start_stop_abort.html
  • So avoid sharing complex data structures that are easily corrupted like double linked lists, and avoid using locks such as mutexs as this may cause your callback function to block and therefore drop audio

I think its misleading to say the ringbuffer is about "helping with performance". I could accept "helping with worst-case performance", but they both imply this is an optimisation step as opposed to a domain requirement.

To be fair these concerns are primarily talking about pthread style mutexes which invoke the OS scheduler. I'm not sure if go's sync.Mutex uses that kind of mutex under the hood or has different characteristics.

I'll concede that accessing the map concurrently without locks is not safe - I haven't looked at the map implementation in detail but I guess the consequence if something rebalances the map while the callback is trying to look it up is it might not find the entry for the given key? Which is just as bad as missing the deadline since we can't determine which stream callback to invoke.

I don't have any better idea at the moment. Something other than a map might work without locks (eg. a series of fixed-size arrays in a linked list?), but seems like it gets complex quickly. I'm not an expert on the go memory model but I've had good results with a lockless ring buffer implementation backed by a fixed size array.

[re. Untrack in Stream.Start] I think it would depend on the type of error no?

I don't think so. http://portaudio.com/docs/v19-doxydocs/terminating_portaudio.html says:

When you are done with a stream, you should close it to free up resources:

ie. the client must call stream.Close to avoid leaking. If they don't it seems perfectly reasonable for us to leak a map entry + Stream reference.

There is currently a missing Untrack in OpenStream if Pa_OpenStream fails. You could also get rid of the previous two Untracks:

• get rid of NewStream
• have Track return the id
• call Track and initialise Stream.id directly before Pa_OpenStream

[jaredfolkins]

Implementation with @sqweek's suggestions. Please review.

jaredfolkins@d559694

[sqweek]

I still don't like the mutex, but I finally ran a test to see how C calling back into go interacts with the scheduler. The test results suggest that if all Ms are in use when the callback is invoked, the calling thread will block (even when that calling thread was not created by go).

ie. the callback approach is already burdened with significant realtime pitfalls. Which is not an argument for adding more, but the mutex is at least easier to manage in the common case where a single Stream is active at once.

So I'm not going to insist on a different solution and would be happy to see this patch merged. I left a couple of final comments (nitpicks really) on d559694.

[jaredfolkins]

Changes made, finalized PR sent.

#11

[mbaroukh]

Hi.
I know I'm not on the right place to ask for this, but I can't find any other way to contact you.
I had a question in case you may take a few linutes to answer :
I'm trying to use this binding and it works well on linux/macos.
But on windows, using mingw, the "#cgo pkg-config: portaudio-2.0" doesn't work.
Are you really using pkg-config ? If not how could you compile on windows ?
I tried also to modify portaudio.go with fhings like
#cgo CFLAGS: -I/z/native/libs/win/include -mthreads
#cgo LDFLAGS: -L/z/native/libs/win/bin -L/z/native/libs/win/lib -lportaudio -lwinmm -lm -lole32 -luuid

but no luck.
using CGO_CFLAGS works for gcc but at linking it complain taht it can't find libportaudio.
I tried CGO_LDFLAGS, LD_LIBRARY_PATH, ... but without success.

I'm not familiar with windows compilation (and with "C" compilation globally .. I'm more form Java world) so maybe I missed something obvious.

Thanks if you can help me.
And if not, thanks anyway for your contribution which allowed me to use GO 1.6 ;)

Mike

[sqweek]

@mbaroukh Yeah pkg-config can be stupidly annoying to get going on windows. Try searching for pkg-config-lite - it has less dependencies and you should be able to just drop the exe into your mingw bin/ dir. If that doesn't work please shoot me an email rather than spamming this thread - sqweek at gmail.

[mbaroukh]

Thanks for your reply @sqweek ! I'll ty.
And this message is my last spam of this thread :)

[gordonklaus]

Sorry for my silence, I only noticed this issue today; my notification settings must be screwed up. Anyway, this should be fixed now.