#2651 - Add thread-safe FSUIPC integration by ghingres · Pull Request #2655 · MobiFlight/MobiFlight-Connector

ghingres · 2026-01-23T17:08:09Z

Fix bug #2651 (FSUIPC Integration is not thread safe) by changing the following

Use thread safe ConcurrentDictionary in place of Dictionary
Use a Lock to avoid FSUIPCConnection.Process() and setValue() running in parallel threads
Set ActionAtNextProcess to be OffsetAction.Write on any setValue() to avoid any Autosense issues (as advised by Peter of FSUIPC).

Fix bug MobiFlight#2651 FSUIPC Integration is not thread safe by changing the following a) Use thread safe ConcurrentDictionary in place of Dictionary b) Use a Lock to avoid FSUIPCConnection.Process() and setValue() are running in parallel threads c) Set ActionAtNextProcess to be OffsetAction.Write on any setValue() flow to avoid any Autosense issues (as advised by Peter of FSUIPC).

DocMoebiuz · 2026-01-23T20:34:46Z

Thanks for the PR. I will have to take some time to properly think about this.
This code is very old and it has a lot of repetitive parts and is not so greatly structured. Also we have to think about the FSUIPC Helper Class which is used by the FSUIPCInputAction.

DocMoebiuz · 2026-01-23T20:35:21Z

I will let Copilot start with a review and see what it thinks, but I would really try to avoid copy/past repetition.

DocMoebiuz

This PR needs unit tests.

This class requires refactoring. Too many DRY violations.

Also check FSUIPCHelper.

DocMoebiuz · 2026-01-23T20:35:47Z

FSUIPC/Fsuipc2Cache.cs

+        ConcurrentDictionary<Int32, Offset<Byte>> __cacheByte = new ConcurrentDictionary<Int32, Offset<Byte>>();
+        ConcurrentDictionary<Int32, Offset<Int16>> __cacheShort = new ConcurrentDictionary<Int32, Offset<Int16>>();
+        //ConcurrentDictionary<Int32, Offset<UInt16>> __cacheUShort = new ConcurrentDictionary<Int32, Offset<UInt16>>();
+        ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();
+        //ConcurrentDictionary<Int32, Offset<UInt32>> __cacheUInt = new ConcurrentDictionary<Int32, Offset<UInt32>>();
+        ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();
+        ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();
+        //ConcurrentDictionary<Int32, Offset<UInt64>> __cacheULong = new ConcurrentDictionary<Int32, Offset<UInt64>>();
+        ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();
+        ConcurrentDictionary<Int32, Offset<String>> __cacheString = new ConcurrentDictionary<Int32, Offset<String>>();


I think this is a good idea.

DocMoebiuz · 2026-01-23T20:37:35Z

FSUIPC/Fsuipc2Cache.cs

+                    lock (fsuipc_lock)
+                    {
+                        FSUIPCConnection.Process();
+                    }


I would introduce a method ProcessThreadSafe that wraps this call to avoid repetition of

lock (fsuipc_lock) { FSUIPCConnection.Process(); }

and just call "ProcessThreadSafe()"

this applies to all subsequent calls.

DocMoebiuz · 2026-01-23T20:46:42Z

FSUIPC/Fsuipc2Cache.cs

+            lock (fsuipc_lock)
            {
-                __cacheByte[offset] = new Offset<Byte>(offset);
-                _offsetsRegistered = true;
-            }
+                if (!__cacheByte.ContainsKey(offset))
+                {
+                    __cacheByte[offset] = new Offset<Byte>(offset);
+                    _offsetsRegistered = true;
+                }

-            __cacheByte[offset].Value = value;
+                ((Offset<Byte>)__cacheByte[offset]).ActionAtNextProcess = OffsetAction.Write;
+                __cacheByte[offset].Value = value;
+
+                //long retval = this.getValue(offset, 1);
+                //if (retval != value)
+                //{
+                //    Log.Instance.log($"Recovered SetOffset() failure: Byte offset {offset}, value was {retval} after requesting {value}", LogSeverity.Warn);
+                //    __cacheByte[offset].Value = value;
+                //}
+            }


With ConcurrentDictionary this is not necessary. ConcurrentDictionary is already thread-safe.
We have to check which methods are the correct ones to add additional keys to the Dictionary

@DocMoebiuz - The original problem (as demonstrated by the commented out code) was occuring when I used ConcurrentDictionary. Even though threadsafe, the method believed it's setvalue was loaded when in fact a parallel FSUIPCConnection.Process() had overidden it's value ( The commented out code would fire and recover the situation). This was the testcase for proving where the locks were required. The setValue method and the FSUIPCConnection.Process() must not be operating in parallel. Peter confirmed the FSUIPCConnection.Process() is a synchonous process.

DocMoebiuz · 2026-01-23T20:51:02Z

I totally missed the

Set ActionAtNextProcess to be OffsetAction.Write on any setValue() to avoid any Autosense issues (as advised by Peter of FSUIPC).

... that's interesing, I was not aware of that. Does that mean that it ONLY performs writes that are pending for the next process?

Copilot

Pull request overview

This pull request aims to fix bug #2651 by adding thread-safety to the FSUIPC integration layer. The changes address race conditions that could cause values to be lost when multiple threads simultaneously read and write FSUIPC offsets.

Changes:

Converted Dictionary collections to ConcurrentDictionary for thread-safe storage of FSUIPC offset objects
Added locks around all FSUIPCConnection.Process() and FSUIPCConnection.SendControlToFS() calls to prevent concurrent FSUIPC operations
Set ActionAtNextProcess = OffsetAction.Write in all setOffset methods to ensure values are written on next Process call, as recommended by FSUIPC author

Copilot · 2026-01-23T20:58:33Z

FSUIPC/Fsuipc2Cache.cs

+                //long retval = this.getValue(offset, 2);
+                //if (retval != value)
+                //{
+                //    Log.Instance.log($"Recovered SetOffset() failure: Short offset {offset}, value was {retval} after requesting {value}", LogSeverity.Warn);
+                //    __cacheShort[offset].Value = value;
+                //}


Similar commented-out debugging code. Either remove it or add a comment explaining its purpose.

Copilot · 2026-01-23T20:58:34Z

FSUIPC/Fsuipc2Cache.cs

+                if (!__cacheInt.ContainsKey(offset))
+                {
+                    __cacheInt[offset] = new Offset<Int32>(offset, writeOnly);
+                    _offsetsRegistered = true;
+                }


Similar thread safety issue. Use TryAdd method instead: __cacheInt.TryAdd(offset, new Offset<Int32>(offset, writeOnly)).

Copilot · 2026-01-23T20:58:34Z

FSUIPC/Fsuipc2Cache.cs

+                if (!__cacheDouble.ContainsKey(offset))
+                {
+                    __cacheDouble[offset] = new Offset<double>(offset);
+                    _offsetsRegistered = true;
+                }


Similar thread safety issue. Use TryAdd method instead: __cacheDouble.TryAdd(offset, new Offset<double>(offset)).

Copilot · 2026-01-23T20:58:34Z

FSUIPC/Fsuipc2Cache.cs

+                }

-            __cacheShort[offset].Value = value;
+                ((Offset<Int16>)__cacheShort[offset]).ActionAtNextProcess = OffsetAction.Write;


Similar to the Byte case, the cast to Offset is unnecessary. Consider removing it.

Suggested change

((Offset<Int16>)__cacheShort[offset]).ActionAtNextProcess = OffsetAction.Write;

__cacheShort[offset].ActionAtNextProcess = OffsetAction.Write;

Copilot · 2026-01-23T20:58:34Z

FSUIPC/Fsuipc2Cache.cs

+                    _offsetsRegistered = true;
+                }
+
+                ((Offset<double>)__cacheDouble[offset]).ActionAtNextProcess = OffsetAction.Write;


Similar to other cases, the cast to Offset is unnecessary. Consider removing it.

Copilot · 2026-01-23T20:58:44Z

FSUIPC/Fsuipc2Cache.cs

+        ConcurrentDictionary<Int32, Offset<Byte>> __cacheByte = new ConcurrentDictionary<Int32, Offset<Byte>>();
+        ConcurrentDictionary<Int32, Offset<Int16>> __cacheShort = new ConcurrentDictionary<Int32, Offset<Int16>>();
+        //ConcurrentDictionary<Int32, Offset<UInt16>> __cacheUShort = new ConcurrentDictionary<Int32, Offset<UInt16>>();
+        ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();


Field '__cacheInt' can be 'readonly'.

Suggested change

ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();

private readonly ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();

Copilot · 2026-01-23T20:58:44Z

FSUIPC/Fsuipc2Cache.cs

+        //ConcurrentDictionary<Int32, Offset<UInt16>> __cacheUShort = new ConcurrentDictionary<Int32, Offset<UInt16>>();
+        ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();
+        //ConcurrentDictionary<Int32, Offset<UInt32>> __cacheUInt = new ConcurrentDictionary<Int32, Offset<UInt32>>();
+        ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();


Field '__cacheFloat' can be 'readonly'.

Suggested change

ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();

readonly ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();

Copilot · 2026-01-23T20:58:44Z

FSUIPC/Fsuipc2Cache.cs

+        ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();
+        //ConcurrentDictionary<Int32, Offset<UInt32>> __cacheUInt = new ConcurrentDictionary<Int32, Offset<UInt32>>();
+        ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();
+        ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();


Field '__cacheLong' can be 'readonly'.

Suggested change

ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();

readonly ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();

Copilot · 2026-01-23T20:58:44Z

FSUIPC/Fsuipc2Cache.cs

+        ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();
+        ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();
+        //ConcurrentDictionary<Int32, Offset<UInt64>> __cacheULong = new ConcurrentDictionary<Int32, Offset<UInt64>>();
+        ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();


Field '__cacheDouble' can be 'readonly'.

Suggested change

ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();

private readonly ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();

Copilot · 2026-01-23T20:58:45Z

FSUIPC/Fsuipc2Cache.cs

+        ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();
+        //ConcurrentDictionary<Int32, Offset<UInt64>> __cacheULong = new ConcurrentDictionary<Int32, Offset<UInt64>>();
+        ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();
+        ConcurrentDictionary<Int32, Offset<String>> __cacheString = new ConcurrentDictionary<Int32, Offset<String>>();


Field '__cacheString' can be 'readonly'.

Suggested change

ConcurrentDictionary<Int32, Offset<String>> __cacheString = new ConcurrentDictionary<Int32, Offset<String>>();

readonly ConcurrentDictionary<Int32, Offset<String>> __cacheString = new ConcurrentDictionary<Int32, Offset<String>>();

@DocMoebiuz - The FSUIPC Client DLL action (This property can be used to force this offset to be read or written on the next process) is documented at http://fsuipc.paulhenty.com/Reference/html/classFSUIPC_1_1Offset.html. Paul suggested different group names for read v write Offsets however this would require Mobiflight to segregate usage of FSUIPC offsets. The work-around was to set the Action at next process.

I'm new to modifying code within open source projects. Is there any documentation/advice on how to perform CoPilot code reviews and also why refactoring is considered a better way ? Usually the first action of anyone is a source code compare to understand changes and any refactoring would result in a mass of such notifications ! Perhaps refactoring should be a 2nd step process after initial code supply ?

This PR has been tested in my environment, the commented out debug(s) assisted in proving the issue and that locking resolved it before being commented out. I can add additional comments as to their purpose but again it helps on any future fixes what standards any developers should be adhering to.

Thanks again for all your support

Gary

DocMoebiuz · 2026-02-05T10:24:41Z

@ghingres will you be able to check the comments and copilot feedback and implement them?

ghingres requested a review from DocMoebiuz as a code owner January 23, 2026 17:08

DocMoebiuz requested changes Jan 23, 2026

View reviewed changes

DocMoebiuz requested review from DocMoebiuz and Copilot January 23, 2026 20:48

Copilot started reviewing on behalf of DocMoebiuz January 23, 2026 20:49 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

	((Offset<Int16>)__cacheShort[offset]).ActionAtNextProcess = OffsetAction.Write;
	__cacheShort[offset].ActionAtNextProcess = OffsetAction.Write;

	ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();
	private readonly ConcurrentDictionary<Int32, Offset<Int32>> __cacheInt = new ConcurrentDictionary<Int32, Offset<Int32>>();

	ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();
	readonly ConcurrentDictionary<Int32, Offset<Single>> __cacheFloat = new ConcurrentDictionary<Int32, Offset<Single>>();

	ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();
	readonly ConcurrentDictionary<Int32, Offset<Int64>> __cacheLong = new ConcurrentDictionary<Int32, Offset<Int64>>();

	ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();
	private readonly ConcurrentDictionary<Int32, Offset<Double>> __cacheDouble = new ConcurrentDictionary<Int32, Offset<Double>>();

	ConcurrentDictionary<Int32, Offset<String>> __cacheString = new ConcurrentDictionary<Int32, Offset<String>>();
	readonly ConcurrentDictionary<Int32, Offset<String>> __cacheString = new ConcurrentDictionary<Int32, Offset<String>>();

Uh oh!

Conversation

ghingres commented Jan 23, 2026

Uh oh!

DocMoebiuz commented Jan 23, 2026

Uh oh!

DocMoebiuz commented Jan 23, 2026

Uh oh!

DocMoebiuz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DocMoebiuz Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

DocMoebiuz Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

DocMoebiuz Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

ghingres Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

DocMoebiuz commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

ghingres Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

DocMoebiuz commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DocMoebiuz left a comment •

edited

Loading