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

Hack to work around a race in Smack. #816

Merged

Conversation

JonathanLennox
Copy link
Member

(See commit 3f57099 for an explanation of the race.)

(See commit 3f57099 for an explanation of the race.)
@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #816 (7133253) into master (3f57099) will decrease coverage by 0.38%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #816      +/-   ##
============================================
- Coverage     45.09%   44.70%   -0.39%     
+ Complexity      761      750      -11     
============================================
  Files           111      111              
  Lines          6511     6527      +16     
  Branches        918      919       +1     
============================================
- Hits           2936     2918      -18     
- Misses         3168     3195      +27     
- Partials        407      414       +7     
Impacted Files Coverage Δ
...ava/org/jitsi/impl/protocol/xmpp/ChatRoomImpl.java 0.00% <0.00%> (ø)
...itsi/protocol/xmpp/AbstractOperationSetJingle.java 54.18% <0.00%> (-1.68%) ⬇️
...jicofo/conference/ParticipantChannelAllocator.java 43.79% <0.00%> (-1.46%) ⬇️
...tsi/jicofo/conference/JitsiMeetConferenceImpl.java 43.70% <0.00%> (-1.43%) ⬇️
.../java/org/jitsi/jicofo/conference/Participant.java 58.75% <0.00%> (-0.57%) ⬇️
...cofo/conference/colibri/ColibriConferenceImpl.java 54.64% <0.00%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3f57099...7133253. Read the comment docs.

bgrozev
bgrozev previously approved these changes Oct 1, 2021
occupantsMapField.setAccessible(true);

Map<EntityFullJid, Presence> occupantsMap = (Map<EntityFullJid, Presence>)occupantsMapField.get(muc);
occupantsMap.clear();
Copy link
Member

Choose a reason for hiding this comment

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

What about printing a log if there was something in the map?

Field occupantsMapField = null;
try
{
occupantsMapField = muc.getClass().getField("occupantsMap");
Copy link
Member

Choose a reason for hiding this comment

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

This should be getDeclaredField

@damencho
Copy link
Member

damencho commented Oct 2, 2021

I did some automation and reverted the previous change and not I cannot hit it ... it is so strange ... I even add in prosody code to send a self presence update before the unavailable message and I see it in jicofo xmpp logs but still nothing, I ran 50 attempts ... will leave it now to do more attempts during the day.
I'm waiting for a print I added on if (muc.getOccupants().size() > 0) or at least the test will fail to join the muc and I will see that ...

@damencho
Copy link
Member

damencho commented Oct 2, 2021

Ok, here is an update. Out of 200 runs managed to repro it once and the fix with clearing the map worked, no locking.
The runs were without prosody modifications and just with reverted the latest change in master about the setting of bridge 0.

@damencho
Copy link
Member

damencho commented Oct 2, 2021

And leaving just the fix from Friday I was able to repro in around 60 attempts. So I think we need this one :)

@JonathanLennox JonathanLennox merged commit d714d83 into jitsi:master Oct 4, 2021
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.

3 participants