Client library deletes pointers without any notification to the calling application.

[gnif]

jack2/common/JackLibGlobals.h

Lines 109 to 118 in ba28ffa

	// Cleanup remaining clients
	jack_error("Jack server was closed but clients are still allocated, cleanup...");
	for (int i = 0; i < CLIENT_NUM; i++) {
	JackClient* client = JackGlobals::fClientTable[i];
	if (client) {
	jack_error("Cleanup client ref = %d", i);
	client->Close();
	delete client;
	}
	}

As the shutdown handler is to be treated like a signal, the cleanup of other connected clients must be deferred, however, if a recovery routine is attempted for a disconnected client, the above code destroys all the client pointers pulling the rug out from under the calling application. There is no way to determine if this "cleanup" has happened and that the old handles are now invalid.

Instead of deleting the objects, this should instead set a flag/state marking the objects as no longer valid, allowing the calling application to properly clean up.

[falkTX]

Do you have some code where this is triggered?

[gnif]

https://github.com/qemu/qemu/blob/master/audio/jackaudio.c

Each client is created per virtual audio device, some have inputs, some have outputs, some have both. The life cycle of each virtual device is completely isolated and independent of the next. Restarting the application due to a Jack server failure is not acceptable as it means restarting the entire guest VM, as such reconnection/recovery of each client/connection is required on a client by client basis.

When the first client recovers and reconnects, it invalidates the client references that are still in use by the other devices which are expecting to clean up and also recover.

In short, not only is the above "cleanup" not documented anywhere, it goes against the standard design pattern of a client library and makes it impossible for the calling application to rely on the handles provided. At the very least the jack library should not delete the objects, keeping them valid, even if they have been cleaned up.

Edit: This code was introduced in 171a3c4, it seems it was added to fix bugs in applications written by third parties. Obviously I will need to work around this by not trying to close the old connections at all, which goes against all common programming conventions.

If this is fixed, please be sure to include a way to determine if this code is removed so that a client that properly implements the API can properly clean up (ie, jack version define)

[gnif]

Perhaps another option would be on connection to set a flag to specify the client is well behaved which disables this cleanup.

Please also note that this issue makes jack2 incompatible with jack1 as it appears that jack1 does not perform this "cleanup".

[falkTX]

I am unsure what to do about this.
I think I understand why this is there. in the case of the server being restarted, jack2 wants to cleanup itself on the client side so it can reconnect cleanly.

@sletz under what circumstances was this needed?

@gnif I would like to have a proper solution that does not rely on adding something just to fix server-side mishaps. A special flag (I guess during registration?) feels exactly like that, though I cant really think of anything better.
Also, I am not sure if simply skipping the deleting of the client pointer is enough here, as the client object might reference stuff that is deleted on the next few lines.

[sletz]

This commit was done 12 years ago and the commit log is somewhat clear:
Client and library global context cleanup in case of incorrect shutdown handling (that is applications not correctly closing client after server has shutdown) .
I don't remember the exact use case but it was quite real. I guess this possible issue is a quite usual server/client model possible behaviour: @gnif what is the correct way to do it then?

[gnif]

Don't do any cleanup at all, just flag the object as "dead" and allow the application to clean up directly as per the normal alloc/free pattern. IMO the above-referenced code simply should be removed. It's not your library's job to work around a badly designed client.

[sletz]

Then I would vote for keeping client->Close(); since 1) it probably deallocate some stuff in shared memory and has to be done for the server to properly start again later on, and remove delete client; to avoid creating this dangling pointer (but at the price of a memory leak).
@falkTX I think you should check that 1) is actually true by writing a test case that simulates this in case of incorrect shutdown handling (that is applications not correctly closing client after server has shutdown) comment.

[falkTX]

I have not had time to check on this, and today was the day set for the new release.
Going to pursue this afterwards.
My idea is to check if the client pointer destructor does anything, and trigger that during this part of cleanup too. also invalidate all pointers, leaving the client pointer valid but not able to do anything.

PR would be appreciated, otherwise I will try to fix this myself in some days/weeks.