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

Bug fix: XOutputDevice.RefreshInput() is not thread safe. #818

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

kenh6942
Copy link

Originally observed issue:

  • When moving 2 joysticks on 2 different input devices mapped to 1 XOutput device at the same time, occasionally one of the joystick inputs would be lost/disconnect.
  • In game, this caused the joystick to remain pressed in some last-polled direction.
  • This could be resolved in the XOutput app by hitting the "Force Refresh" button.

Debug/conclusion process:

  • Log would show a "WARN Poll failed for ..." when this occurs.
  • Log event is from DirectDevice.cs line 319 (logger.Warning($"Poll failed for {ToString()}");)
  • The exception thrown to cause the log event was a "Collection was modified ..."
  • Exception was thrown from line 313 (InputChanged?.Invoke(this, deviceInputChangedEventArgs);)
  • ...which was invoking XOutputDevice.SourceInputChange() which then calls RefreshInput()
  • Debugging calls in the function, it appeared to be the call to changes.Any() that was throwing the exception.
  • Looking online for other instances of when such an exception could occur, the quick solution is to make a copy of the Collection for iteration. Adding ToList() to line 119 (changes.GetChanges(force);) appeared to fix the issue, but is more masking the problem than fixing it.
  • How is the Collection being changed? Each DirectDevice object that eventually calls into XOutputDevice.RefreshInput() has a thread. RefreshInput() makes changes to the state variable of XOutputDevice. It is possible for more than one thread to be calling RefreshInput() and thus making asynchronous changes to the state variable.
  • Excluding it's initialization, state is only used/of-use in RefreshInput().
  • Solution: Make state a local variable of RefreshInput().

This does mean a DeviceState is instantiated for each call to RefreshInput() which is inefficient.

@kenh6942
Copy link
Author

Originally branched off the 3.32 tag. The forced push was to rebase on top of 3.x branch.

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.

1 participant