A couple of potential problems with CJamRecorder

[cdmahoney]

I've recently been playing around with Jamulus code (implementing inter process communication through an mqtt broker) and have come across a couple of potential problems with the implementation of CJamRecorder (they are not bugs at the moment, but could become so in the future.)

1. CJamRecorder constructor does not initialize member variable currentSession of type CJamSession*. The variable is initialized in CJamRecorder::Start and set to nullptr in CJamRecorder::OnEnd, but any comparison of the pointer with nullptr before a call to CJamRecorder::Start will return false (for example, if CJamRecorder::OnDisconnected is called before CJamRecorder::Start, which can't happen with the current Jamulus source code, but which could occur if the call to JamController.GetRecordingEnabled before emitting ClientDisconnected is removed from CServer::DecodeReceiveData, which is what my modified code does.)
2. ChIdMutex will not be unlocked after a call to ChIdMutex.lock if the calling method returns prematurely - for example, a return statement or exception within the code block between the calles to ChIdMutex.lock and ChIdMutex.unlock. QMutexLocker can be used to gain a lock which will always be freed when the enclosing block goes out of scope. Currently CJamRecorder::OnDisconnected contains a nested return statement which will be called if currentSession == nullptr resolves to true, returning from the method without releasing the lock.

As stated above, while the code currently works fine, if at some point CServer::DecodeReceiveData is modified to not call JamController.GetRecordingEnabled, both of these cases will provoke errors - the first will provoke a segmentation fault when a user disconnects if recording is not enabled, and if the first case is fixed the second case will provoke an permanent lock on the mutex in the same situation.

I'd be happy to make pull requests with the changes in the code for jamrecorder.h and jamrecorder.cpp to fix the issues.

Apologies if this is not the correct way to submit this potential issue.

[softins]

Thanks for the report! It’s always good to make code more robust, and we would welcome a PR for the improvements.

We are preparing to release 3.8.0 in the coming days. As the points you mention are not currently bugs, could I suggest that you wait until after the release, and then submit your PR against the head of master at the time?

[softins]

Would also be interested in what this MQTT broker could do for Jamulus. Perhaps you would like to open a discussion?

[cdmahoney]

OK, will make pull request after 3.8.0 is released - will give me time to swot-up on how to do so (I've not much experience of github)

Will also open discussion of using mqtt broker with jamulus. Basic idea is to provide simple inter process communication in a platform-independant way. This would make jamulus extensible, without having to add code to jamulus for every use-case that someone can come up with. Currently my code sends messages for status updates (user connected/disconnected, recording started/stopped,...) and the idea is for jamulus to also listen for messages so the external applications can for example start and stop recordings. So for example a web application could connect to jamulus by mqtt, provide information about currently connected users, enable recording etc, without having to use signals or start services - Vincenzo Della Mea's Jamulus Recording application for example could probably use this functionality. I'll provide a more complete (and clearer!) description on the discussion.

[cdmahoney]

Discussion created: #1789

[softins]

OK, will make pull request after 3.8.0 is released - will give me time to swot-up on how to do so (I've not much experience of github)

There is a description of the usual Github workflow in the file TRANSLATING.md - just ignore the parts about doing actual translation, and think about doing code changes at the same points instead.

There is also a summary at #1762 (comment)

[cdmahoney]

There is a description of the usual Github workflow in the file TRANSLATING.md - just ignore the parts about doing actual translation, and think about doing code changes at the same points instead.

There is also a summary at #1762 (comment)

OK, thanks - I'll look that up.

[ann0see]

@cdmahoney I'll close this issue since it seems to be fixed now here: #1826

If this is not the case, please re-open this issue.