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

Race condition with data items' initialization caused sporadic test failures #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SergeyGulik
Copy link

Patrick, thank you a lot for supporting the tests I added. It makes very easy for me migration to a newer version of MTConnect.NET since I can always refer to the updated tests in your repository.

Please read a problem description below. If you have any better solution than applied, please share it with me.

Adapter client initialized all its data items, setting them to UNAVAILABLE
Lest creating race condition we should wait until all items are initialized in OnConnect (see ShdrClient.ListenForAdapter)
The essence of race condition was that a data item might be sent via adapter EARLIER than adapter client initialized the same data item to UNAVAILABLE.
As a result MTConnectAgent.FilterPeriod was discarding new value coming via adapter due to older timestamp assigned on sending.
I have chosen ShdrClient.PingSent event, since ShdrClient.Listening was not called.

…LABLE

Lest creating race condition we should wait until all items are initialized in OnConnect (see ShdrClient.ListenForAdapter)
The essence of race condition was that a data item might be sent via adapter EARLIER than adapter client initialized the same data item to UNAVAILABLE.
As a result MTConnectAgent.FilterPeriod was discarding new value coming via adapter due to older timestamp assigned on sending.
I have chosen ShdrClient.PingSent event, since ShdrClient.Listening was not called.
@PatrickRitchie
Copy link
Contributor

@SergeyGulik I'm glad to hear you are able to migrate changes easily and keep up to date with the latest version.

I'll have to take a look at this issue further. It looks like I may need to add a check to see what the current dataitems are and only send Unavailable if there is no value for the dataitem already in the adapter's current list. The main concern would be to make sure that there isn't a gap in the data and to notify a client application that there was a loss of connection between the agent and the adapter.

Thank you for continuing to create tests and pull requests. This really helps cover issues that I have missed during development. Please let me know if you find anything else.

Thanks,
-Patrick

@PatrickRitchie
Copy link
Contributor

After looking into this I think the best solution is to not set UNAVAILABLE in the ShdrAdapterClient.OnConnect() method and to set a new timestamp in the SendLast() method. This will prevent the scenario you described from happening as the new dataitem will always have a newer timestamp than the initial UNAVAILABLE as shown below:

Agent Restart

All DataItems are initialized as UNAVAILABLE (sequence 149). Upon connection to the Adapter, the last value is sent by the adapter (sequence 210).
image

Adapter Restart

Upon Adapter diconnection, an UNAVAILABLE observation is added (sequence 230). Upon Adapter reconnection, the last value is sent by the adapter (sequence 407), and subsequent new observations are added as normal (sequences 408-410).
image

I don't think that the UNAVAILABLE should have ever been set in the ShdrAdapterClient.OnConnect() method as there shouldn't be a scenario that this is needed. Only the OnDisconnect() method should set UNAVAILABLE.

I also added a timestamp parameter to the SendLast() methods so that all of the latest observations can have the same timestamp upon reconnection.

I believe all of this conforms to the functionality described in Part 1 : Section 5.1.3.7 of the MTConnect Standard as shown below:

image

If anyone disagrees or has any other feedback on how this should work then please let me know. Otherwise I will add the changes described above and include them in the next release.

Thanks,
-Patrick

@PatrickRitchie
Copy link
Contributor

I included the updates I mentioned above in the latest v1.1.0 release. If anyone finds any issues with this behavior then please let me know.

Thanks,
-Patrick

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.

None yet

3 participants