search

jamulussoftware / jamulus

Jamulus enables musicians to perform real-time jam sessions over the internet.
https://jamulus.io
Other
997 stars 222 forks source link

A couple of potential problems with CJamRecorder #1788

Closed cdmahoney closed 3 years ago

cdmahoney commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

cdmahoney commented 3 years ago

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 commented 3 years ago

Discussion created: https://github.com/jamulussoftware/jamulus/discussions/1789

softins commented 3 years ago

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 https://github.com/jamulussoftware/jamulus/pull/1762#issuecomment-850810137

cdmahoney commented 3 years ago

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 commented 3 years ago

@cdmahoney I'll close this issue since it seems to be fixed now here: https://github.com/jamulussoftware/jamulus/pull/1826

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