Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CJamRecorder: Add currentSession pointer initialization and improve locking #1826

Merged
merged 2 commits into from
Jun 6, 2021

Conversation

cdmahoney
Copy link

  • Added initialization of currentSession pointer in CJamRecorder constructor
  • Changed locking code in CJamRecorder to use QMutexLock, to prevent leaving mutex locked if method exits with exception or nested return statement.

Server code tested on Windows and Ubuntu 20.04.

@ann0see
Copy link
Member

ann0see commented Jun 4, 2021

Thanks for opening this PR!

@pljones should have a look at this code since he wrote most of it.

@hoffie hoffie changed the title Changes to CJamRecorder CJamRecorder: Add currentSession pointer initialization and improve locking Jun 4, 2021
Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jamulus uses an odd code style. You should apply clang format ;-).

src/recorder/jamrecorder.h Outdated Show resolved Hide resolved
src/recorder/jamrecorder.cpp Outdated Show resolved Hide resolved
src/recorder/jamrecorder.cpp Outdated Show resolved Hide resolved
src/recorder/jamrecorder.cpp Outdated Show resolved Hide resolved
src/recorder/jamrecorder.cpp Outdated Show resolved Hide resolved
Copy link
Member

@hoffie hoffie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
On a high level, this looks good to me. I'm leaving the more extensive reviews for @pljones and @softins though.

There's one formatting thing to fix (you can run clang-format on it if you have version 10 or later). I've just approved the workflows for this PR which will probably also show the relevant line.

src/recorder/jamrecorder.h Outdated Show resolved Hide resolved
@hoffie hoffie requested review from pljones and softins June 4, 2021 20:39
@hoffie hoffie added this to the Release 3.8.1 milestone Jun 4, 2021
Copy link
Member

@softins softins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, subject to the formatting adjustments that have already been mentioned. Thanks!

Copy link
Collaborator

@pljones pljones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@cdmahoney
Copy link
Author

OK, will upload corrected file later - after running clang-format! Thanks to all for the helpful comments.

Copy link
Member

@ann0see ann0see left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not tested personally; code style seems to be ok. I'll let @pljones or someone else merge it.

@pljones pljones merged commit 7dcd207 into jamulussoftware:master Jun 6, 2021
@hoffie
Copy link
Member

hoffie commented Jun 6, 2021

CHANGELOG: Jam Recorder's internal locking and initiatlization code has been made more robust.

@pljones
Copy link
Collaborator

pljones commented Jun 7, 2021

Now running on my jam server.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A couple of potential problems with CJamRecorder
5 participants