#923 Remove ConsoleWriterFactory by pljones · Pull Request #926 · jamulussoftware/jamulus

pljones · 2021-01-30T14:16:03Z

All instances of ConsoleWriterFactory have been removed.

In addition, on Windows (when not headless), the console has been attached once on start up to ensure messages appear. This only happens when run from the command line - otherwise the messages don't appear.

Parameterised messages use QString formatting but that's needed an extra qUtf8Printable call to pass the result to qInfo() etc.

A few of the start up messages were warnings, so they're now qWarning(), rather than qInfo(). Start up failure is now displayed as qCritical() messages.

As we've generally moved to qInfo() etc, I removed the QT version check where it was in place.

Unrelated bits...

I changed the help text for --centralserver to use the term "server list".
I fixed the formatting of --ctrlmidich and --clientname to be consistent with the other long opt-only options.

pljones · 2021-01-30T14:23:13Z

Tests:

tested start up with every parameter (-v last) and all the messages formatted ok
started a linux server and a Windows client and connected to it - all messages formatted ok
sent a "start new recording" signal to the server - messages formatted ok
sent a "toggle recording" signal to the server - messages formatted ok
registered a server at a serverlist (found a bug...) - messages formatted ok (now)

Anything else anyone can think of?

pljones · 2021-01-31T12:12:56Z

@softins @ann0see - can you review?

softins · 2021-01-31T12:52:40Z

Will do, but it will either be later today or else tomorrow.

ann0see · 2021-01-31T12:54:00Z

I can look into it tomorrow

pljones · 2021-01-31T14:16:33Z

Thanks :)

ann0see

It's quite a big change. It does compile...

I think, the only way to see if it really fixed the memory leak is to try it on a server.

Nevertheless, your changes look good.

ann0see · 2021-02-01T07:35:00Z

src/main.cpp

    QString      strWelcomeMessage           = "";
    QString      strClientName               = "";

+#if !defined(HEADLESS) && defined(_WIN32)


I assume this is the first time we change coding style?

ann0see

Still to be tested (real world test), but since you couldn't find issues, I will approve it.

Memory usage does increase (it was at 1.1 MB and after having connected & disconnected multiple clients enabled and disabled the recorder). It now stays at 1.5 MB on my Debian 10 laptop

softins · 2021-02-01T12:28:55Z

I saw in several places the comment // TODO we should use the ConsoleWriterFactory() instead of qInfo(), so I'm trying to understand the reasons for firstly that comment, and now the opposite decision. I see ConsoleWriteFactory says it was implemented by @pljones

What environments has compilation been tested on so far? I can test on Linux(RPi) and Mac(Mojave), but not (yet) PC.

ann0see · 2021-02-01T12:31:50Z

The CodeQL check makes sure that it compiles on all 3 OS. So this should be ok.

Raspberry Pi is untested but I‘m quite sure if it compiled on Debian and Ubuntu (both amd64) armhf should work too.

softins · 2021-02-01T12:34:49Z

The CodeQL check makes sure that it compiles on all 3 OS. So this should be ok.

I have much to learn! Not sure what this is.

Raspberry Pi is untested but I‘m quite sure if it compiled on Debian and Ubuntu (both amd64) armhf should work too.

Just trying this now.

softins

Compiles and runs fine on RPi too, so approving.

Would still like to understand the reason for originally moving to ConsoleWriterFactory and why this no longer applies.

softins · 2021-02-01T12:52:39Z

It would appear that my review doesn't count - presumably I don't have write access:

At least 2 approving reviews are required by reviewers with write access.

ann0see · 2021-02-01T13:10:42Z

Does it work now? I just changed the role maindeveloper role to have write access and made you an organisation admin.

I think we can merge this now

softins · 2021-02-01T13:23:45Z

Does it work now? I just changed the role maindeveloper role to have write access and made you an organisation admin.

Thanks, github is now happy with my approval.

I think we can merge this now

I agree.

ann0see · 2021-02-01T13:26:00Z

Merged. @pljones I think we should still do some real world tests.

ann0see · 2021-02-01T13:53:20Z

I have much to learn! Not sure what this is.

It basically tests if the code compiles and to some extent checks it for vulnerabilities.

pljones · 2021-02-01T21:28:28Z

Would still like to understand the reason for originally moving to ConsoleWriterFactory and why this no longer applies.

Qt4 compatibility. We gave up on that some time last year, I think.

pljones · 2021-02-01T21:43:56Z

Merged. @pljones I think we should still do some real world tests.

Before the push I did

./Jamulus -n -i foo.ini -p 55850 -t -d -e localhost -f '[1.2.3]' -l /dev/stdout -L -m /dev/stdout -o 'here;here;23' -R /tmp --norecord -s -T -u 1 -w 'Hello world' -z -M --mutemyown -c 1 -j --ctrlmidich 13 --clientname foo -l /dev/stdout

and it seemed to work okay -- all the main.cpp messages appeared - also adding -v stopped start up still.

I also did

./Jamulus -n -p 55850 -d -e localhost -f '[3.0.0]' -R /tmp -s -T -u 1 -c 1

and tested the various recorder messages with SIGUSR1, SIGUSR2 and clients connecting and servers registering / unregistering.

softins · 2021-02-01T23:53:23Z

Would still like to understand the reason for originally moving to ConsoleWriterFactory and why this no longer applies.

Qt4 compatibility. We gave up on that some time last year, I think.

Ah, cool, understood. Thanks! I remember that happening.

@pljones

@pljones If this fixed the memory leak, please add it here.

pljones added 2 commits January 30, 2021 14:13

Remove ConsoleWriterFactory

89cdff6

Remove ConsoleWriterFactory

e1b11dd

pljones mentioned this pull request Jan 30, 2021

Memory leak in the server #923

Closed

ann0see reviewed Feb 1, 2021

View reviewed changes

ann0see self-requested a review February 1, 2021 12:11

ann0see approved these changes Feb 1, 2021

View reviewed changes

softins approved these changes Feb 1, 2021

View reviewed changes

ann0see merged commit da07b89 into jamulussoftware:master Feb 1, 2021

pljones deleted the feature/#923-remove-console-textstream branch February 1, 2021 21:28

ann0see added a commit that referenced this pull request Feb 2, 2021

Document #926

373e1f4

@pljones If this fixed the memory leak, please add it here.

pljones added this to the Release 3.7.0 milestone Nov 24, 2024

Comments

Conversation

pljones commented Jan 30, 2021

Uh oh!

pljones commented Jan 30, 2021

Uh oh!

pljones commented Jan 31, 2021

Uh oh!

softins commented Jan 31, 2021

Uh oh!

ann0see commented Jan 31, 2021

Uh oh!

pljones commented Jan 31, 2021

Uh oh!

ann0see left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ann0see Feb 1, 2021

Choose a reason for hiding this comment

Uh oh!

ann0see left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

softins commented Feb 1, 2021

Uh oh!

ann0see commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins commented Feb 1, 2021

Uh oh!

softins left a comment

Choose a reason for hiding this comment

Uh oh!

softins commented Feb 1, 2021

Uh oh!

ann0see commented Feb 1, 2021

Uh oh!

softins commented Feb 1, 2021

Uh oh!

ann0see commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ann0see commented Feb 1, 2021

Uh oh!

pljones commented Feb 1, 2021

Uh oh!

pljones commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

softins commented Feb 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ann0see left a comment •

edited

Loading

ann0see left a comment •

edited

Loading

ann0see commented Feb 1, 2021 •

edited

Loading

ann0see commented Feb 1, 2021 •

edited

Loading

pljones commented Feb 1, 2021 •

edited

Loading