Repository navigation
Stability Issues Addressed #88
Replies: 8 comments 13 replies
|
Hi @pnpebeling I can then review it, and get it merged into the v4 branch? |
|
Sure ... What I have is preliminary, I'd like it to run a while longer to be sure of the potential fixes. When I'm a bit more confident, I'll clean it up and do the Pull. -p |
|
@pnpebeling, can I ask you what was the exact exception related to the Callbacks dictionary you saw? What was the reason your PR #89 was not merged? |
|
That should have been merged, happy to include it. Sorry, I must have not gotten to it, and the branch was deleted after - my bad @pnpebeling can you raise a new PR against main, if you both still have a reference to it? |
|
why wont that be needed in v5 ? v5 still has the Callbacks dictionary right ? |
|
Sorry I'm referring to the
|
|
Spudwebb,
I believe the ConcurrentDIctionary is a solid fix. As to the
SendInternalSynchronized ... it may or may not be necessary. I added it
when chasing the concurrency issues (probably solved by the
ConcurrentDictionary). There was a comment in the SendInternalSynchronized
api warning about simultaneous calls ... so I added the code just in case.
…-p
On Tue, Feb 3, 2026 at 11:31 AM Marcus Davies ***@***.***> wrote:
That should have been merged, happy to include it.
V5 wont need such fix, but happy to patch the current branch (as it still
represents V4).
Sorry, I must have not gotten to it, and the branch was deleted after - my
bad
@pnpebeling <https://github.com/pnpebeling> can you raise a new PR
against main, if you both still have a reference to it?
—
Reply to this email directly, view it on GitHub
<#88 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIIA4WG22QFOX7Q7CPG4WQD4KDSQRAVCNFSM6AAAAACCD2ONX6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTKNRYGY2DKMA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Do you remember by any chance the type of exception you got before the fix? |
|
I haven't looked at v5, but I'm assuming it will need it (the
ConcurrentDictionary) as well. The SendInternalSynchronized probably not,
because as I understand it v5 uses a different web interface.
As for creating a new pull request ... I am woefully github ignorant.
Maybe someone else can look at the closed PRs and revive it. It's an
easy fix. ... sorry.
…-p
On Tue, Feb 3, 2026 at 11:49 AM spudwebb ***@***.***> wrote:
why wont that be needed in v5 ? v5 still has the Callbacks dictionary
right ?
—
Reply to this email directly, view it on GitHub
<#88 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIIA4WH7KTPI5FCH4U3WV6T4KDUP3AVCNFSM6AAAAACCD2ONX6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTKNRYGY3TGNI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I'll look up the code change around the dictionary, and implement it in V5 |
|
I had created a custom routine to retrieve node neighbors. As I recall I'd
get "index-not-found" errors around the Callbacks dictionary upon
completion. It smelled like some form of corruption, and when I dug deeper
I came across the Callbacks dictionary. My application puts a lot of
multi-threaded pressure on the ZWave stuff, exposing the issue quite
readily. Your mileage may vary in single threaded or low pressure
implementaitons.
…-p
On Tue, Feb 3, 2026 at 12:03 PM spudwebb ***@***.***> wrote:
Do you remember by any chance the type of exception you got before the fix?
—
Reply to this email directly, view it on GitHub
<#88 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIIA4WCFY3AADBMJ34HUYED4KDWGRAVCNFSM6AAAAACCD2ONX6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTKNRYGY4TEMI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
The SendInstant lock code was added simply based on the api comment ...
/// <summary>
/// Send text message to the websocket channel.
/// It doesn't use a sending queue,
/// beware of issue while sending two messages in the exact same time
/// on the full .NET Framework platform
/// </summary>
/// <param name="message">Message to be sent</param>
I was chasing what appeared to be concurrency issues, and this comment made
me suspicious. I did not test this lock independently of the
ConcurrentDictionary code, so it may or may not be necessary.
…-p
On Tue, Feb 3, 2026 at 12:32 PM Marcus Davies ***@***.***> wrote:
Sorry I'm referring to the SendInstant use.
The patch for the dictionary will be added in V5 (still a WIP)
—
Reply to this email directly, view it on GitHub
<#88 (reply in thread)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIIA4WGD5BX6SMVUW6OUIUT4KDZVHAVCNFSM6AAAAACCD2ONX6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTKNRYG4YTKMY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
If you have both received the error - I shall patch the dictionary with the PR rasied originally (still not sure that happened there). |
Sorry - I have quite the vocabulary 😅 |
|
@marcus-j-davies , any progress on this? Do you want me to create a PR ? |
|
Sorry @spudwebb - Im lacking progress on this repo
Please do - Target |
|
Merged, Im not at my dev machine currently but will publish to NUGET once at the workhorse. (and thank you both ) |
|
I'm good, I applied the packages locally.
thanks
…-p
On Tue, Feb 3, 2026 at 1:05 PM Marcus Davies ***@***.***> wrote:
@spudwebb <https://github.com/spudwebb>, @pnpebeling
<https://github.com/pnpebeling>
If you have both received the error - I shall patch the dictionary with
the PR rasied originally (still not sure that happened there).
Do you both still have a reference to the main branch?
—
Reply to this email directly, view it on GitHub
<#88 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIIA4WCATXF5LXLIR7JBGC34KD5RJAVCNFSM6AAAAACCD2ONX6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTKNRYG4ZTGNA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
That's the error, 99% sure the CD fix will solve it.
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: spudwebb ***@***.***>
Sent: Tuesday, February 3, 2026 1:20:43 PM
To: zwave-js/ZWaveJS.NET ***@***.***>
Cc: Paul Ebeling ***@***.***>; Mention ***@***.***>
Subject: Re: [zwave-js/ZWaveJS.NET] Stability Issues Addressed (Discussion #88)
I have not see the error myself, but one user of my app reported an "Index was outside the bounds of the array" error when sending a lot of commands at the same time, and I suspect the Callbacks dictionary to be the cause.
Not sure what you mean by having a reference to the main branch? my fork is in sync with the main branch.
—
Reply to this email directly, view it on GitHub<#88 (reply in thread)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AIIA4WGMOCDBZJH66VTN2W34KD7JXAVCNFSM6AAAAACCD2ONX6VHI2DSMVQWIX3LMV43URDJONRXK43TNFXW4Q3PNVWWK3TUHMYTKNRYG42DGMA>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Uh oh!
There was an error while loading. Please reload this page.
Hi,
I'm experiencing stability issues which I believe are related to thread safety. During extended run testing I have experienced exceptions related to the (Driver) Callbacks Dictionary and (what I believe are) some web-socket related hangs, though I have no "smoking-gun" to prove this theory. I have addressed these issues as follows:
The Callbacks dictionary is defined as a simple Dictionary Dictionary<Guid, Action> which provides no thread safety. Since this construct is referenced both when callbacks are added and completed, I felt that some form of thread safety was required. I have implemented a replacement class that wraps a CurrentDictionary exposing only the required Dictionary methods.
As to the (presumed) web-socket hangs, I noticed the use of the SendInstant method. The comments warn of issues with simultaneous access. Since my calling framework is multi-threaded, I made a simple change to the WebcocketClient, SendInstant methods to force synchronization around a lock object.
Its fairly early in my testing, but these changes seem to have addressed the stability issues.
-p
All reactions