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

Fixing concurrent dispatchTo crash #6362

Merged
merged 1 commit into from
Jun 22, 2022
Merged

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Jun 22, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes #5821 & #6080

  • Uses a safe to concurrently modify structure for the SessionListeners list

Motivation and context

We have a few implementations of the SessionListener which remove themselves from the SessionListeners in onSessionStopped (SessionScopedProperty, InvitesAcceptor) , this is a problem because we iterate through all the listeners in order to call onSessionStopped, resulting in a concurrent modification exception.

The error itself seems to be race condition presenting itself intermittently due to synchronised usages allowing the next main looper to be used, I'm only able to reproduce locally by directly including a listener which removes itself ASAP but the crash is quite common in the Play console

// Session.close()
sessionListeners.addListener(object : Session.Listener {
    override fun onSessionStopped(session: Session) {
        sessionListeners.removeListener(this)
    }
})

The fix is to use a structure which allows concurrent modification, in this case a CopyOnWriteArraySet which does have an extra performance cost but at the point of modification (rather than iteration) which we don't do very often.

Screenshots / GIFs

Before After
before-dispatchto after-dispatchto

Tests

  • In the code, add a new session listener which removes itself onSessionStopped
  • Notice a crash when signing out

Tested devices

  • Physical
  • Emulator
  • OS version(s): 28

- whilst closing the session, we iterate through all the listeners calling onSessionStopped however some implementations also use this callback to remove themselves from the listeners, causing the concurrent modification crash
@ouchadam ouchadam added the PR-Small PR with less than 20 updated lines label Jun 22, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jun 22, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@bmarty bmarty self-requested a review June 22, 2022 12:56
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ouchadam
Copy link
Contributor Author

for such a tiny change going to merge with 1 👍

@ouchadam ouchadam merged commit 9925cf9 into develop Jun 22, 2022
@ouchadam ouchadam deleted the bugfix/adm/dispatch-to-crash branch June 22, 2022 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Small PR with less than 20 updated lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signing out causes a crash
2 participants