Skip to content
This repository has been archived by the owner on Dec 3, 2024. It is now read-only.

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 kenh6942 force-pushed the multi-stick-disconnect-fix branch from a5e9894 to 2b86eda Compare January 14, 2024 19:14
@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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant