# OSCdef/Func: Adding a function prevents the responder from being cleared/freed. #3285

Open
opened this Issue Nov 9, 2017 · 22 comments

Projects
None yet
5 participants
Contributor

### mtmccrea commented Nov 9, 2017 • edited by telephon

 Adding a function to both OSCdef and OSCFunc will prevent the responder from being cleared or freed, though it is removed from the AbstractResponderFunc.allFuncProxies list. I'm on SuperCollider 3.9.0-beta1, but I've seen the problem since at least 3.8 Observe: ( // create an OSCdef with an original function d = OSCdef(\testOSC, { |msg, time, addr, recvPort| "ORIGINAL function".postln; }, '/ping', nil ); ) AbstractResponderFunc.allFuncProxies['OSC unmatched'].do(_.postln) // there it is AbstractResponderFunc.allFuncProxies['OSC unmatched'].size n = NetAddr("localhost", 57120) n.sendMsg('/ping') // responds, OK // now free the oscdef d.free n.sendMsg('/ping') // doesn't respond, because it's freed // all cleaned up... AbstractResponderFunc.allFuncProxies['OSC unmatched'].do(_.postln) // re-create an OSCdef with an original function ( d = OSCdef(\testOSC, { |msg, time, addr, recvPort| "ORIGINAL function".postln; }, '/ping', nil ); ) // now add a function f = {|msg, time, addr, recvPort| "FIRST added function".postln;} d.add(f) n.sendMsg('/ping') // added function responds // clear the OSCdef d.clear n.sendMsg('/ping') // it's still executing the functions // free the OSCdef d.free n.sendMsg('/ping') // it's still executing the functions // yet... AbstractResponderFunc.allFuncProxies['OSC unmatched'].do(_.postln) // no longer on the funcProxies list !! // need to recompile now to get rid of that headless responder :(
Contributor

### jamshark70 commented Nov 10, 2017

 99% chance it's the dispatcher's problem, likely: https://github.com/supercollider/supercollider/blob/develop/SCClassLibrary/Common/Control/ResponseDefs.sc#L147
Contributor

### jamshark70 commented Nov 10, 2017 • edited

 Workaround: Don't use 'add'. Just make another OSCdef. Problem: When you 'add' the function, the OSCdef's function changes into a FunctionList. After that point, the OSCBlahBlahDispatcher doesn't know how to replace the FunctionList with new responders, or nil. ( d = OSCdef(\testOSC, { |msg, time, addr, recvPort| "ORIGINAL function".postln; }, '/ping', nil); ) z = OSCdef(\testOSC).dependants.detect(_.notNil); z.slotAt(\active).at('/ping'); -> a Function d.free; z = OSCdef(\testOSC).dependants.detect(_.notNil); z.slotAt(\active).at('/ping') -> nil  ... and nothing responds. But: ( d = OSCdef(\testOSC, { |msg, time, addr, recvPort| "ORIGINAL function".postln; }, '/ping', nil); ) f = {|msg, time, addr, recvPort| "FIRST added function".postln;} d.add(f); z = OSCdef(\testOSC).dependants.detect(_.notNil); z.slotAt(\active).at('/ping'); -> a FunctionList z.slotAt(\active).at('/ping').array.do(_.postcs); { |msg, time, addr, recvPort| "ORIGINAL function".postln; } {|msg, time, addr, recvPort| "FIRST added function".postln;} d.free; z = OSCdef(\testOSC).dependants.detect(_.notNil); z.slotAt(\active).at('/ping'); -> a FunctionList  ^^ and it's still there. Case of mistaken Identity, I guess. (This, btw, is why I avoid the FunctionList interface in my own code. It's Clever. Clever usually = bugs. I know this from painful experience.)
Member

### telephon commented Nov 10, 2017 • edited

 [OT] There are both addFunc and removeFunc. Where did you run into problems?
Contributor

### jamshark70 commented Nov 10, 2017

 The issue here seems to be with replaceFunc. I have, on a few occasions, used addFunc where I needed multiple responses to, say, GUI actions. I haven't had specific problems but I think it's because I keep my usage deliberately simple (and I keep it that way because I don't fully trust the way a Function can magically morph into a FunctionList and back again). The usage in the osc dispatchers seems not simple, and indeed has a problem.
Contributor

### muellmusik commented Nov 10, 2017

 I'll have a look...

Contributor

### mtmccrea commented Nov 12, 2017

@jamshark70 the problem indeed appears to be in the handling of FunctionList:

In AbstractWrappingDispatcher:remove:

Line 141 in 769f78c

 keys.do({|key| active[key] = active[key].removeFunc(func) }); // support multiple keys

In the case of functions that have been added, when active[key] returns a FunctionList. FunctionList:removeFunc expects a Function, and when it receives instead a FunctionList, it can't properly remove the functions from the list, and/or set that active[\key] to nil, as it does when removing a function:

Line 456 in 769f78c

 removeFunc { arg function; if(this === function) { ^nil } }

This may also be a problem in:

Line 147 in 769f78c

 updateFuncForFuncProxy {|funcProxy|

when when trying to replace the FunctionList .

@telephon it appears addFunc and add are not interchangeable. When using addFunc in the original example, the added function isn't executed with the original, as it is with add.

Like @jamshark70 I usually end up opting for a simpler solution, adding new responders, or the this.changed/update scheme, but I've been meaning to report this because it reminded me of the OSCresponderNode days when it was easier to accumulate headless responders.

Thanks for having a look!

Member

### telephon commented Nov 13, 2017

 @mtmccrea yes, this is documented and intended behavior. The rationale was that when you remove something from a list, you know already what you remove, and you don't need to return it. What you return is the new storage object, and in this case it just goes back to nil (nil is like a fixed point of the whole interface, that is the intention).
Contributor

### muellmusik commented Nov 13, 2017

 In the case of functions that have been added, when active[key] returns a FunctionList. FunctionList:removeFunc expects a Function, and when it receives instead a FunctionList, it can't properly remove the functions from the list, and/or set that active[\key] to nil, as it does when removing a function: I'm still not quite sure I see where the gotcha is. You should be able to nest FunctionLists in other FunctionLists. If what's stored in active and wrappedFuncs is correct and the same, then it should work AFAICS. Sorry, just coming off a big project and probably still a bit dazed. If you've really found it could one of you explain in small words for a bear of simple brain, and point.
Member

### telephon commented Nov 13, 2017

 yes, you can nest FunctionLists. But you shouldn't expect that removing X from Y will remove all function inside X also from Y. I'm not sure if that is what is the problem.
Contributor

### muellmusik commented Nov 13, 2017

 @telephon If that is the problem, I would expect double triggers
Member

### telephon commented Nov 13, 2017

 Maybe the reason is that clear  is called instead of free?  free { allFuncProxies.remove(this); this.disable } clear { this.prFunc_(nil) }
Contributor

### jamshark70 commented Nov 13, 2017

 Note carefully, in my analysis, I used free but the functions were not cleared from the dispatcher. That isn't the problem.
Contributor

### muellmusik commented Nov 13, 2017

 Okay, this will take some more detailed investigation. I'm on the road for the next few days, so will be next week...
Contributor

### jamshark70 commented Nov 14, 2017

 OK, I think this is the problem. AbstractWrappingDispatcher:remove and :updateFuncForFuncProxy assume that the object under active[key] matches the structure of the parent OSCFuncs / OSCdefs. That is: ( d = OSCdef(\testOSC, { |msg, time, addr, recvPort| "ORIGINAL function".postln; }, '/ping', nil); ) f = {|msg, time, addr, recvPort| "FIRST added function".postln;} d.add(f); d.func -> a FunctionList  The two methods I cited both assume that any FunctionList under a given key will contain objects that are identical to the funcs belonging to any OSCFunc/defs registered to that key. In the test case, we have one OSCdef with two functions, so, to meet that assumption, the object structure in the dispatcher should be: FunctionList([ FunctionList([originalFunc, firstAddedFunc]) ])  Instead, the actual object structure is simply FunctionList([originalFunc, firstAddedFunc]). So, either one of these solutions would do it: The dispatcher's add method could ensure a nested object structure corresponding exactly to the parents' structures. Or, add should always create a flat FunctionList, and remove / updateFuncForFuncProxy should unpack FunctionLists found at the parent level. I'm guessing the flat FunctionList strategy would be simpler and harder to break (but you'd have to test the case of OSCFunc(FunctionList([ ... ]), ...) -- i.e., the dispatcher's add method may have to unpack FunctionLists too, and ensure no duplicates).
Contributor

### jamshark70 commented Nov 14, 2017 • edited

 Here's a test demonstrating the object structure problem (with a couple of debug statements added into the class library): ( d = OSCdef(\testOSC, { |msg, time, addr, recvPort| "ORIGINAL function".postln; }, '/ping', nil); ) f = {|msg, time, addr, recvPort| "FIRST added function".postln;} d.add(f); calling replaceFunc: [ /ping, a Function, a FunctionList ] -> OSCdef(testOSC, /ping, nil, nil, nil)  So replaceFunc is replacing the old single Function with a FunctionList (containing two functions). d.free; calling removeFunc: [ /ping, a FunctionList ] -> OSCdef(testOSC, /ping, nil, nil, nil)  I.e., it's doing theDispatchersFunctionList.removeFunc(aFunctionList)... but theDispatchersFunctionList just has a flat array of two functions. That's not going to work.
Contributor

### muellmusik commented Nov 14, 2017

 I.e., it's doing theDispatchersFunctionList.removeFunc(aFunctionList)... but theDispatchersFunctionList just has a flat array of two functions. That's not going to work. And yet: f = FunctionList(); f.addFunc(g = {\bar}) f.array -> [ a Function ] f.replaceFunc(g, h = FunctionList()) f.array -> [ a FunctionList ] f.replaceFunc(h, g); f.array -> [ a Function ]  If replaceFunc returned nil when the FunctionList is empty and then added that would make sense (thought we'd found it when that occurred to me) but it's a straight replace on the array.  replaceFunc { arg find, replace; array = array.replace(find, replace); } 
Contributor

### muellmusik commented Nov 14, 2017

 Okay, got it. It's not a flat array of two functions, the issue is specifically that it's myFunctionList.replaceFunc(myFunctionList, nil). This should be easy to fix. I can do it in the dispatcher, but I think possibly FunctionList should handle the special case of replacing itself? Or maybe have a flag or additional method. f = {|msg, time, addr, recvPort| "FIRST added function".postln;}; f.replaceFunc(f, nil); -> nil g = 2; g.replaceFunc(g, nil); -> nil // and yet h = FunctionList(); h.replaceFunc(h, nil); -> error as h's array is nil h = FunctionList(); h.addFunc({\foo}); h.replaceFunc(h, nil); -> a FunctionList  This needs a little thought. (What if the FunctionList contains itself? Well that's a hang as soon as you call value anyway) @telephon ?
Contributor

### jamshark70 commented Nov 14, 2017

 Okay, got it. It's not a flat array of two functions, the issue is specifically that it's myFunctionList.replaceFunc(myFunctionList, nil). That's not exactly it. Hang on a minute, I'm writing up another example.
Contributor

### jamshark70 commented Nov 14, 2017

 Aha, turns out, we're both partially right. Let's walk through another case, step by step. d = OSCFunc({ "f1".postln }, '/ping'); -> OSCFunc(/ping, nil, nil, nil) f = d.dependants.detect(_.notNil); -> an OSCMessageDispatcher f.slotAt(\active).at('/ping') -> a Function  d's func is a single function, and the corresponding dispatcher's /ping slot contains a single function. d.add({ "f2".postln }); d.func; -> a FunctionList f.slotAt(\active).at('/ping') -> a FunctionList f.slotAt(\active).at('/ping').array -> [ a Function, a Function ]  Now, it happens that d's func and the dispatcher's function list are identical. Now let's up the ante: e = OSCFunc({ "f3".postln }, '/ping'); e.func; -> a Function f.slotAt(\active).at('/ping').array -> [ a Function, a Function, a Function ]  The dispatcher now has a flat list of three functions. BUT... here's another bug, which I didn't expect to find: d has now been corrupted. Because it shares the FunctionList reference with the dispatcher, d now contains e's function as well. d.func.array -> [ a Function, a Function, a Function ] d.func.array.collect { |func| func.def.constants } -> [ [ f1 ], [ f2 ], [ f3 ] ]  You might never notice, because the dispatcher is the object that actually calls the functions, and it has only one copy of each function. But this object structure is not correct. The user did not add a third function to d. Now, if you do d.free, then it is what Scott said: myFunctionList.replaceFunc(myFunctionList, nil). myFunctionList is not a member of itself (for good reason), so nothing gets removed. I think possibly FunctionList should handle the special case of replacing itself. No. I think first, the object structure bug needs to be fixed, because currently it's impossible to free d without also freeing e. Two valid options: Flat dispatcher structure d.func = FunctionList([a Function, a Function]) e.func = a Function Dispatcher @ /ping = FunctionList([a Function, a Function, a Function]) Then, d.free would do removeFunc once for each member of its own function list. Mirrored dispatcher structure d.func = FunctionList([a Function, a Function]) e.func = a Function Dispatcher @ /ping = FunctionList([FunctionList([a Function, a Function]), a Function]) Then, d.free would do removeFunc for its own two-member function list.
Contributor

### jamshark70 commented Nov 14, 2017

 Alternate recommendation: Remove add and remove from the responder interfaces. They aren't necessary and they introduce complications that are difficult to manage. Just dump them. One OSCFunc, one function, period, end of discussion, no nested nuthin'.
Member

### telephon commented Nov 14, 2017

 i was just about to suggest this ...
Contributor

### muellmusik commented Nov 14, 2017

 I think I'd be okay with that. I guess that's a deprecate? We could eventually simplify things slightly behind the scenes because of that.