feat: Offload xmpp/smack processing threads. #500
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Make sure we do not send and/or wait for responses in the smack threads.
Codecov Report
@@ Coverage Diff @@
## master #500 +/- ##
============================================
+ Coverage 20.82% 21.02% +0.20%
- Complexity 286 292 +6
============================================
Files 74 74
Lines 6186 6220 +34
Branches 832 832
============================================
+ Hits 1288 1308 +20
- Misses 4675 4690 +15
+ Partials 223 222 -1
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
More discussions: https://discourse.igniterealtime.org/t/thread-stuck-in-multiuserchat-enter-forever/92090/15?u=flow |
bgrozev
reviewed
Sep 26, 2023
| @@ -603,7 +612,7 @@ private synchronized void setXmppProvider( | |||
|
|
|||
| if (xmppProvider.isRegistered()) | |||
| { | |||
| joinConferenceRoom(); | |||
| xmppExecutorPool.execute(this::joinConferenceRoom); | |||
There was a problem hiding this comment.
joinConferenceRoom() itself has one big try catch, that should catch everything...
| * A thread pool used to offload xmpp execution in a new thread to avoid blocking | ||
| * xmpp threads. | ||
| */ | ||
| public static final ExecutorService xmppExecutorPool = Util.createNewThreadPool("xmpp-executor-pool"); |
There was a problem hiding this comment.
Do we not have a generic "io" thread pool in jigasi?
There was a problem hiding this comment.
Nothing, this becomes one for the xmpp handling.
| @@ -1081,6 +1100,11 @@ public void serviceChanged(ServiceEvent serviceEvent) | |||
|
|
|||
| @Override | |||
| public void memberPresenceChanged(ChatRoomMemberPresenceChangeEvent evt) | |||
| { | |||
| xmppExecutorPool.execute(() -> memberPresenceChangedInternal(evt)); | |||
There was a problem hiding this comment.
These may now execute concurrently or out of order. Do they need to go on a queue?
There was a problem hiding this comment.
I don't think that matters ...
| @@ -624,7 +624,7 @@ public void notifyChatRoomMemberJoined(ChatRoomMember member) | |||
| public void notifyChatRoomMemberLeft(ChatRoomMember member) | |||
| { | |||
| // if this is the sip hanging up (stopping) skip playing | |||
| if (gatewaySession.getJvbConference().isStarted() | |||
| if (gatewaySession.getJvbConference() != null && gatewaySession.getJvbConference().isStarted() | |||
There was a problem hiding this comment.
AbstractGatewaySession.jvbConference can change to null, so this can still produce an NPE (granted, rarely)
bgrozev
approved these changes
Sep 27, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure we do not send and/or wait for responses in the smack threads.