-
Notifications
You must be signed in to change notification settings - Fork 222
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
Newly connected empty clients don't show up on old clients #2754
Comments
@softins can you help out with some potentially network (server) related? |
Not until next week. I'm away on holiday until Saturday. You could send details through in the meantime if you want. |
Ok. Great no problem. You should also have received my E-Mail with the logs and a issue description. hoffie could not reproduce the problem, I could on multiple OS. |
@pljones Softins has a proposed fix (sent via mail). Could you please look over it too (if you’re ok with it? |
Not received... and not marked as spam. |
Ok. Will forward it |
A new connection must always cause a channel update signal to be emitted, even if the channel info for the new channel is empty. Fixes jamulussoftware#2754
A new connection must always cause a channel update signal to be emitted, even if the channel info for the new channel is empty. Fixes jamulussoftware#2754
Excerpt: [Privacy issue]: Newly connected empty clients don't show up on old clientsPlease note that the conversation needs to be read from the bottom to the top! pljones (DC/Matrix) My only concern is why this hasn't had a very noticeable effect. Why is it an edge case? I think we need to have a fully documented scenario saying how to reproduce the behaviour and why this change must (logically) be the fix. The code, as code, looks okay. The code, as code, before the change, looked okay, too. As of 3.8.0 we don't have a connection until the client info comes in. So the Reset happens on disconnect. If no connect happens (with an emit now always), the channel should just sit empty unused. I'd rather the emit happened and wasn't needed than it didn't happen and was needed... Some chat on Matrix/Discord with pljones followed. Excerpts are following ann0see (Matrix/Discord) softins Thanks ! All, I have pushed my fix to my own repo at https://github.com/softins/jamulus/tree/fix-new-empty-chaninfo if anyone wants to try it. Let me know when I should raise a PR (although after today, I'm probably not free again until Sunday). Cheers (Cc direct to , since he might not be receiving the team emails?) hoffie Thanks for spotting and fixing this, and ! The proposed fix sounds good to me. <...> ann0see: Yes, that looks like a possible fix. I’d like to have Peter‘s OK here too (and maybe do a local test) then you can open a PR and Admin merge it. softins:
Please let me know how best to proceed. softins My new idea is much simpler, does not need any magic in CChannelCoreInfo, and addresses the problem right where it occurs.
I have built a server with this change, and unnamed clients are now all displayed as they should be. softins OK, I have a better idea, which I will test first and then describe. ann0see Hi , softins Just to follow this up, it probably makes most sense to specify that an empty name always compares not equal, since this is the least likely to happen in practice. Also, we would need to test both operands, so that the != operator was commutative:
I’ll test this change and if the team are happy with the approach, I can raise a PR.
The variable CChannel::ChannelInfo is of type CChannelCoreInfo, which is defined in src/util.h, and initialised according to the constructor there.
The test ( eSkillLevel == SL_NOT_SET ) could just as easily be one of the other members, depending on which is considered most appropriate:
so won’t be able to look at it till tomorrow. If anyone else wants to have a look, feel free!
I cleared out the default “No Name” from the client name, but initially didn’t change the country from the default United States. I could not reproduce the problem. Both clients showed both channels. I then changed the country from United States to None, and could then reproduce the problem:
As soon as I request the welcome message via explorer or someone joins, I see the muted symbol. Therefore I would conclude that the empty client only shows up as soon as the client list gets resent (I assume it doesn’t get sent on the join of an empty client although it gets sound).
Not yet tested, I assume it’s been like this for many versions. softins Hi all, As I said on the placeholder issue, I can’t look properly <...>. But I had a few initial thoughts:
Cheers |
Re-opened as there's still post discussion needed. @pljones ? |
Also adding the following here as it's pertinent:
What this fix does is somehow avoid Based on "standard" operation "create"/"update"/"read"/"delete", we want to trigger sending the full channel list whenever any operation on a single channel happens, other than "read". What this change has done is overload "update" by adding an extra field ( |
I'm only unhappy because, whilst it might work, there are high level design issues... |
That may or may not be the case. The PR #2774 fixing the current issue is not intended to make any design changes; it is just making sure the existing design continues to work under one particular edge case. To outline the current design as intended by Volker:
Now, in 99.999% of cases, the above operations work correctly. After 3.9.0 was released, it was discovered that if a client had been configured with an empty name and a country of “None” in its profile (and ONLY in this case), the initial channel info it sends in step 5 failed the “info has changed” check in step 2 above, because it was identical to a newly-initialised So the PR uses the existing That means there is no edge case, and the current design above now works correctly in all cases. The other clients are all informed of the new channel list. If the underlying design can be improved, that should be the subject of separate discussion and does not negate the value and necessity of the current fix at this moment. The reason the behaviour was not observed when connecting to a server older than 3.5.5 was because at 3.5.5 (commit 726cc6f) a backward-compatibility call was removed by Volker as no longer necessary, which had directly called the function mentioned in 3 above. |
Just to add: I've tagged a nightly. We should decide how to proceed (publish a new release, publish an announcement for recommended updating,...). |
I'd recommend updating to anyone operating a post-3.5.5 server that's open to the public. (Not that most registered servers will update - they've been on the same version since they were installed - but for the few that are maintained, it's worth the announcement.) |
So I guess by now it's clear that this fix will be part of a shortly released 3.9.1. I've already added a reminder to the release tracker. I'm therefore closing this issue for now. |
A new connection must always cause a channel update signal to be emitted, even if the channel info for the new channel is empty. Fixes jamulussoftware#2754
Communication started on 24 Jul 2022
This is an excerpt of an E-Mail conversation
Since it might be a privacy issue, I'm reporting it per Mail.
Filler for: #2754
Description of bug:
If two users with an empty name join a server, the first user does not
see the join of the second user on the mixer board. This might be a
privacy issue.
*Expected behavior:
Both users should see the user without name on their mixer board
Screenshots:
C1 is left, C2 is right
To reproduce:
As soon as we e.g. set an instrument in c2, it shows up in c1
Operating System:
Debian 11
Additional context:
Observed in: #2738
The text was updated successfully, but these errors were encountered: