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

Fix Blocked errCh Channel #120

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

Conversation

ty3gx
Copy link

@ty3gx ty3gx commented Aug 31, 2023

Summary:
This PR resolves an issue in the Start function of the monitoring package, specifically concerning how the errCh channel could become blocked. The PR also addresses an array index out-of-bounds issue in the processIndicationFormat1 function.

Issue Details:
* Blocked errCh Channel
In the original implementation, the errCh channel was used to handle errors generated by both the m.streamReader.Recv(ctx) and m.processIndication(ctx, indMsg, m.measurements, m.nodeID) methods. However, the channel can lead to a blocking issue:

To illustrate, when m.processIndication first produces an error, this error is consumed by the parent Start function. As a result, the Start function returns, eliminating the only consumer for the errCh channel. Consequently, when a second error is posted to errCh, the send operation gets indefinitely blocked. This blockage prevents the associated indication processing goroutine from processing subsequent indication messages.

* Array Index Out-of-Bounds in processIndicationFormat1
The processIndicationFormat1 function had the potential to cause a panic due to array index out-of-bounds. This happened when measInfoList was smaller in size than meadDataRecords. A simple check is added to prevent the panic.

Please let me know if these fixes are reasonable or if any further modifications are required.

@onf-bot
Copy link
Contributor

onf-bot commented Aug 31, 2023

Can one of the admins verify this patch?

@gab-arrobo
Copy link
Contributor

ok to test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants