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

UDP echotests fix in case of no memory or device busy. #12333

Merged
merged 1 commit into from
Feb 3, 2020

Conversation

tymoteuszblochmobica
Copy link
Contributor

@tymoteuszblochmobica tymoteuszblochmobica commented Jan 29, 2020

UDP echotests hold in case of memory or device busy status.
This gives possibility of freeing memory and mesh device recover from busy state.

Summary of changes

If Wisun mesh is running and mbed-trace is disabled

UDP burst tests consumes all available memory and sendto fails with NSAPI_ERROR_NO_MEMORY because Nanostack ns_mem_internal_alloc returns NULL buffer.
Problem is with temporary memory lack this fix gives time to memory manager for freeing unused memory pools.

Impact of changes

No

Migration actions required

No

Documentation


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@SeppoTakalo
@AnttiKauppila
@michalpasztamobica

@@ -94,6 +94,9 @@ void UDPSOCKET_ECHOTEST_impl(bool use_sendto)
} else {
sent = sock.send(tx_buffer, pkt_s);
}
if (sent == NSAPI_ERROR_NO_MEMORY) {
Copy link
Contributor

@michalpasztamobica michalpasztamobica Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to do the same for the non-blocking implementation?
And don't we need to check for NSAPI_ERROR_BUSY here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets re-think this..

Your sleep_for() might be fixing the issue, but only because it adds some delay.

But within that delay, there might be other packets coming (duplicates), cumulating our input buffers.

So instead of adding a error handling here, we should follow a generic UDP application priciples:

  1. Ignore send errors.
  2. Don't retry immediately.
  3. Let upper layer or application logic handle the retries and timeouts.

Our application logic is a few lines down. On line 103 on original. There is continue call, which immediately retries. So to fix this, I propose that we remove the continue and allow it to flow directly into the receive() loop that starts from line 106.

Notice that in case the packet did not really go out, we get nothing back, and this loop will restart. So the retry logic is there already. But instead of immediately retrying, it would allow recv() or recvfrom() to drain potential duplicate packets away from input buffers.

So, we should be OK with only one "sleep" within this logic. If this does not work, make sure that SOCKET_TIMEOUT is long enough that we do sleep in recvfrom() and while doing that, drain our input buffers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done as you suggested. Both continue and sleep removed.
It's also working fine.

@ciarmcom
Copy link
Member

@tymoteuszblochmobica, thank you for your changes.
@AnttiKauppila @SeppoTakalo @michalpasztamobica @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@@ -94,6 +94,9 @@ void UDPSOCKET_ECHOTEST_impl(bool use_sendto)
} else {
sent = sock.send(tx_buffer, pkt_s);
}
if (sent == NSAPI_ERROR_NO_MEMORY) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets re-think this..

Your sleep_for() might be fixing the issue, but only because it adds some delay.

But within that delay, there might be other packets coming (duplicates), cumulating our input buffers.

So instead of adding a error handling here, we should follow a generic UDP application priciples:

  1. Ignore send errors.
  2. Don't retry immediately.
  3. Let upper layer or application logic handle the retries and timeouts.

Our application logic is a few lines down. On line 103 on original. There is continue call, which immediately retries. So to fix this, I propose that we remove the continue and allow it to flow directly into the receive() loop that starts from line 106.

Notice that in case the packet did not really go out, we get nothing back, and this loop will restart. So the retry logic is there already. But instead of immediately retrying, it would allow recv() or recvfrom() to drain potential duplicate packets away from input buffers.

So, we should be OK with only one "sleep" within this logic. If this does not work, make sure that SOCKET_TIMEOUT is long enough that we do sleep in recvfrom() and while doing that, drain our input buffers.

This  gives possibility of freeing memory and mesh device recover from busy state.
@tymoteuszblochmobica
Copy link
Contributor Author

Done as Seppo suggested. Both continue and sleep removed.
It's also working fine.

@mergify mergify bot added needs: CI and removed needs: work labels Jan 31, 2020
@jamesbeyond
Copy link
Contributor

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 31, 2020

Test run: SUCCESS

Summary: 5 of 5 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit f7c5822 into ARMmbed:master Feb 3, 2020
@0xc0170 0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 3, 2020
@mergify
Copy link

mergify bot commented Feb 3, 2020

This PR does not contain release version label after merging.

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 3, 2020

Investigating why the label is being not picked up :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants