SegFault on port_registration_callback for existing clients on quit

[MaurizioB]

There is a strange behaviour for clients existing before a new jack.Client is created, once the port registration callback is set. I made a test using python 2.7, ardour and jack_mixer. 1) launch the application; it has to have both audio and midi inputs and outputs. For example, this doesn't happen when launching jack_mixer without any channel set. 2) create a client with a port registration callback; these commands will suffice:

client = jack.Client('xyz', no_start_server=True)
def port_cb(*args):
    print 'port_cb: {}'.format(args)
client.set_port_registration_callback(port_cb)
client.activate()

3) close the former application

This generates a segfault on my machine (unfortunally I'm not able to make tests on other setups). The strange thing is that this does not happen when the application is started after the jack.Client instance is created and activated. This problem is not reproducible when using a client created with jackclient-python (I don't exactly know why, maybe they exit in a different way).

[mgeier]

Thanks for this nicely reproducible bug report!

The problem seems to be that the unregistered port doesn't exist anymore at the point when the callback is invoked.

Before the user-specified callback is called, the function jack_port_by_id() is called, which returns NULL if the port doesn't exist anymore. This is not checked and causes the segfault later.

Apart from the port registration callback, this might also affect the port connect callback. Probably even the port rename callback, if the port is destroyed right after renaming it.

The question is now: what should happen instead of the segmentation fault?

• Call the callback function with None instead of a Port object?
• Silently ignore the event and don't call the callback at all?
• Raise an exception?
• Something else?

[MaurizioB]

You're welcome! By the way, thank you for this module! :) Well, since I'm not a programmer, I'm not really sure what should be the better solution here, so I'm giving you just some ideas as a user who has fun with python.

I think that ignoring the event or raising an exception can be confusing: if I set a callback for that, I'd expect it to behave everytime in the same way, whether the other client's ports have been created before or after. Calling with None can be confusing too, because I wouldn't know why some callbacks would have the Port object and others wouldn't. Consistency should be important.

I just made a small test setting the connection callback too: I started the other client, I activated the jack.Client, I connected/disconnected every port in the other client, and then I close it. No SegFault. So, if I have understood this correctly, I suppose that this issue doesn't happen when the other client is started after jack.Client is activated because the Port objects have already been created and stored in memory (then its reference to the port already exists), wheter it is for registration, connection and, maybe, renaming.

If that's the case, would it be possible (and reasonable) to create Port objects and store their reference on client.activate, whenever a port related callback is set? Does this make sense at all?

Other possibilities:

• Return a "dummy" port object, with properties set to None/0/False, except for the name.
• Call the callback with the port.name instead of the port object everytime a port is unregistered; not so consistent, but at least we would know what to expect when register is False, and theoretically there should be no need to interact with the port object once it's unregistered.
• Use two different callbacks for registration and unregistration, with the latter returning the port.name only

But these suggestions don't consider the correlation with the eventual port connect callback, if that really happens. In that case, I really don't know what would be the better solution.

[mgeier]

Thanks for your input! Even as a non-programmer you can give valuable recommendations about the semantics and API of the library. Probably even better than a programmer who thinks too much about the implementation details ...

Nevertheless, I'm limited by the JACK API, so I'll often have to find a trade-off between the most user-friendly API and something that can actually be implemented with reasonable effort.

I think that ignoring the event or raising an exception can be confusing: if I set a callback for that, I'd expect it to behave everytime in the same way, whether the other client's ports have been created before or after.

I agree, but the callbacks are generated by JACK, and JACK itself shows this inconsistent behavior. I don't see a way how I could fix this.

The only possibility I see for now, is to provide an explicit option to switch behaviors: either call the callback with "invalid" ports or don't call the callback at all. I'd use the "don't call" behavior as a default, because then at least it's guaranteed to always have a valid Port object.

... would it be possible (and reasonable) to create Port objects and store their reference on client.activate, whenever a port related callback is set?

I'm not sure if I understand this suggestion. It would be theoretically possible to store all existing ports on client.activate(), but I could only store the port pointer (jack_port_t*) and not the port ID (jack_port_id_t), which I would need in the callback (in fact, I would need both). There is no function to get the ID from the pointer, only the other way round. Additionally, this wouldn't work because other clients could create new ports after client.activate(), which wouldn't be accounted for. And even if it were possible, I wouldn't want to store the list of ports in my Python library, since that's the job of the JACK library itself.

Return a "dummy" port object, with properties set to None/0/False, except for the name.

I don't like the idea of a "valid" object that's actually invalid. Also, since I cannot get the port pointer (only its ID), I cannot get its name, either. I might as well simply use None, then.

... Call the callback with the port.name instead of the port object ...

Again, that's not possible since I don't have the port pointer. Also, the ability to rename ports makes this very fragile.

... theoretically there should be no need to interact with the port object once it's unregistered.

I guess the idea is that one could want to remove the port from some application-specific list once its unregistered. But since in some cases the port doesn't exist anymore, this is unreliable. One could argue that makes all un-registration callbacks obsolete, but I'm reluctant to remove them completely.

Use two different callbacks for registration and unregistration, with the latter returning the port.name only

Again, port.name is not available without the port pointer. I could make two different callbacks, but I don't see the point. Also, I can only register both at a time, so having two different callbacks would probably be confusing.

I made a possible implementation in #36, what do you think about it?

Although you didn't experience any segfaults with the other two callbacks, I've also added the option there, since according to the JACK API it may theoretically happen there, too.

Does this option (and its default behavior) make sense?

I don't really like the name ignore_unavailable, do you know a better one?

For the record: Did you use JACK 1 or 2? I can reproduce the error with JACK1, but it doesn't seem to happen with JACK2.

[MaurizioB]

I made a possible implementation in #36, what do you think about it? I don't really like the name ignore_unavailable, do you know a better one?

I've seen #36, looks fine to me. What about ignore_invalid instead of ignore_unavailable?

... would it be possible (and reasonable) to create Port objects and store their reference on client.activate, whenever a port related callback is set?

I'm not sure if I understand this suggestion. It would be theoretically possible to store all existing ports on client.activate(), but I could only store the port pointer (jack_port_t*) and not the port ID (jack_port_id_t), which I would need in the callback (in fact, I would need both). There is no function to get the ID from the pointer, only the other way round. Additionally, this wouldn't work because other clients could create new ports after client.activate(), which wouldn't be accounted for. And even if it were possible, I wouldn't want to store the list of ports in my Python library, since that's the job of the JACK library itself.

I see your point. The suggestion was a bit confusing, indeed. What I see is that as soon as a jack.Port object is created, the issue doesn't seem to happen anymore. I don't exactly know why, I just assume that the python interpreter just stores the pointer and doesn't actually call the jack_port_by_id everytime.

I made another simple test right now; I called client.get_ports(), and after that everything was fine, no SegFaults, even with more than 20 ports going offline instantaneously.

What if, instead of adding the ignore argument to the callbacks, client.activate() runs an "internal" get_ports()? I don't know what's the actual jack function you would need, but I can see that the library doesn't need a port to exist anymore, if it's already been "inspected". For example, I was able to get a port from get_port_by_name from a port of a client that was already closed. Which, actually, might be a bug, but, at least, is handy for my needs and indirectly avoids the segfaults.. :)

Anyway, I think I'll go with this simple "workaround", it's easy and doesn't require lots of resources, since I need to call it only on activation, and after that port_registration_callback will do its job.

[mgeier]

Thanks for checking out #36.

What about ignore_invalid instead of ignore_unavailable?

Hmmm, I don't really like either of them. I think the "invalid" part might suggest that something is wrong with the port, but actually it was fine until it was deleted by some other process. I was hoping that "unavailable" wouldn't have this potential for misunderstanding ...

Any other suggestions?

We could also try to remove the double negation and use something like only_existing (which I like even less BTW)?

... I just assume that the python interpreter just stores the pointer and doesn't actually call the jack_port_by_id() everytime.

Yes, when a jack.Port object is created, only the port pointer is stored. I couldn't find any guarantee in the JACK docs about how long this pointer (and its associated metadata like port name etc.) will be valid.

The function jack_port_by_id() is only ever used in the three "port" callbacks. This API suggests that it is possible that the port in question (and its metadata) isn't available anymore in which case the function returns NULL. jackclient-python should never crash the Python interpreter, therefore I'll have to check for this case, like I tried in #36. I think its not safe to assume (without confirmation from the JACK docs) that a deleted port will still be availabe if jack_get_ports() is called on client.activate(). Apart from that, I'm also hesitant to add this call to the library, because it very much smells like a hack.

However, if you feel comfortable that it does the right thing in your case, you can just call client.get_ports() in your Python code. The additional option in the callback registration functions wouldn't hurt in that case (except for one more if clause per callback), right?

[MaurizioB]

(Sorry, I didn't get the notification.) Other ideas for the option: what about "fallback" (True: calls with None, False: doesn't call? Or... "nonetheless" (a bit strange, but it actually makes sense)?

Yes, when a jack.Port object is created, only the port pointer is stored. I couldn't find any guarantee in the JACK docs about how long this pointer (and its associated metadata like port name etc.) will be valid.

Well, I'm active on the #ardour channel on freenode, where Paul Davis (JACK lead developer and one of the devs for Ardour too) usually is. I might ask there as well.

I think its not safe to assume (without confirmation from the JACK docs) that a deleted port will still be availabe if jack_get_ports() is called on client.activate(). Apart from that, I'm also hesitant to add this call to the library, because it very much smells like a hack.

I agree, it's not very elegant and it wouldn't feel right to do that in a library. Still, for my projects works, so I think I'll stick with it as long as there's no other solution. Plus, most of the times, when I need a port callback, usually I would need to call get_ports anyway, so it wouldn't hurt much. As for the additional if, I don't think it's a major problem too; also, we already know we are on a non RT thread, and no RT callback should interfere when a port unregisters.

[mgeier]

Thanks for the further suggestions!

what about "fallback" [...] or "nonetheless"

Hmmm, I don't think I would understand what is meant with this ...

What about only_available=True?

Or inhibit_unavailable=True?

If you hear something from the JACK people, please let me know!

[mgeier]

I've decided to use only_available and merged it into master.

@MaurizioB Thanks again for testing!

[MaurizioB]

Hi, sorry, I was busy and forgot about this. I've asked about that on irc, Davis himself answered me that jack_port_t* is valid upon entering jack_port_unregister, so I suppose that listening clients might or might not get a pointer at all (if they haven't got it before), it depends on how the closing client handles the port unregistration. I think we can safely suppose that when a client unregisters "too many" ports at the same time, the callbacks are not able to "keep the pace", and, in the meantime, the jack server has already removed them. If I understood correctly, it should be safe to assume that every port available upon client.activate will have a valid pointer for jack_get_ports(). I get that "automatically" calling jack_get_ports() on client activation doesn't seem so "elegant", but we could also assume that everytime a user needs to set port callbacks, it's because he needs to track what the jack graph does; I don't see many scenarios for which he wouldn't do that (if not for a very simple port event logger). Since we already know what might happen, if you still don't want to add a get_ports call on activation whenever any port callback is set, at least you might consider suggesting that call in the documentation.

Anyway, glad to be helpful, thank you for this module :)

[mgeier]

@MaurizioB Thanks for the additional info!

I really don't like adding an unconditional call to get_ports() to activate(), because it looks like a hack and most people won't need it.

I might be persuaded to add it to set_port*callback(), would it work there?

It would be even better to just leave it to the user.

Can you please make a PR for adding this to the documentation?

[MaurizioB]

Well, my idea was indeed to add a get_ports only when callbacks are set and if the programmer wants it, but it could be an argument too, something like set_port*callback(callback, get_ports=False).

But then another question might rise: wouldn't this make only_available useless? In that case, the two arguments should be exclusive, maybe, at least for consistency.

[mgeier]

I don't see the point of a separate argument get_ports. If a user wants, she can simply call myclient.get_ports() anywhere in her code.

wouldn't this make only_available useless?

If all ports are "available", it is indeed useless. But I don't think it hurts, either. I think everyone will keep the default only_available=True, but I still think it is good to have this as a visible option, even if it's only for documentation. Without this, the library would (potentially) silently ignore some JACK callbacks, and I don't want to do that behind the backs of my users.

[MaurizioB]

I agree. Well, at least we've found out exactly where the problem is and how to relate with it. I still don't like the unexpected behaviour, but that's JACK's fault, certainly not yours. And I completely understand why you don't like the automatic get_ports call.

I still suggest you to specify all this somewhere in the documentation, though; a note in the port callbacks should suffice, IMHO.

[mgeier]

Can you please create a pull request with the suggested documentation changes?

You can find instructions on how to build the documentation in CONTRIBUTING.rst.

[MaurizioB]

That's it: https://github.com/spatialaudio/jackclient-python/pull/37 Thank you again!