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

DNS send returning WOULD_BLOCK forces delayed retry #9900

Merged

Conversation

Projects
None yet
8 participants
@michalpasztamobica
Copy link
Contributor

commented Mar 1, 2019

Description

In case send returns a WOULD_BLOCK error, we should not retry imediatelly but wait some time.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo
@AriParkkila

@SeppoTakalo
Copy link
Contributor

left a comment

No printf() should be left there.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

I know there is a direct printf call in the code, but before we merge this we should test this against a platform, which actually returns WOULD_BLOCK from send and see that DNS still resolves correctly.
@AriParkkila, would you recommend a platform, where I could see that printf triggerred? I assume it is some cellular modem?

@ciarmcom ciarmcom requested review from AriParkkila, SeppoTakalo and ARMmbed/mbed-os-maintainers Mar 1, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@VeijoPesonen

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I guess mbed-trace would be the way to go here instead of printf.

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I hate traces on normal behaviour, so I'm fine without those.

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

@VeijoPesonen , @SeppoTakalo . I plan to remove that printf, but I need to see it trigger and not crash anything ;-). @AriParkkila , please provide us with an information on which platforms will make use of this (if in doubt - this PR is to follow internal ticket ONME-4181)
@0xc0170 , Travis has failed on a file I did not modify and do not own. How could we go about it?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@0xc0170 , Travis has failed on a file I did not modify and do not own. How could we go about it?

Breakage on master fix, restarted the job

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

Reviews please here!

Show resolved Hide resolved features/netsocket/nsapi_dns.cpp Outdated

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:dns_async_handle_would_block branch Mar 11, 2019

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Mar 11, 2019

features/netsocket/nsapi_dns.cpp Outdated
@@ -30,6 +30,9 @@
#include "PlatformMutex.h"
#include "SingletonPtr.h"

#define TRACE_GROUP "DNS_API"

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Mar 11, 2019

Contributor

Trace group name should be max 4 characters long

features/netsocket/nsapi_dns.cpp Outdated
@@ -1021,7 +1024,15 @@ static void nsapi_dns_query_async_send(void *ptr)
err = query->socket->sendto(dns_addr, packet, len);

if (err < 0) {
query->dns_server++;
if (err == NSAPI_ERROR_WOULD_BLOCK) {
tr_debug("NSAPI_ERROR_WOULD_BLOCK in nsapi_dns_query_async_send\n");

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Mar 11, 2019

Contributor

Is this really needed actually.

This comment has been minimized.

Copy link
@VeijoPesonen

VeijoPesonen Mar 11, 2019

Contributor

In other words, either remove the trace print or fix the trace group name.

@michalpasztamobica michalpasztamobica force-pushed the michalpasztamobica:dns_async_handle_would_block branch to ee056da Mar 11, 2019

@michalpasztamobica

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@0xc0170 , I removed the debug print entirely, as @VeijoPesonen requested. Could you please restart the CI?

@0xc0170

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@0xc0170 , I removed the debug print entirely, as @VeijoPesonen requested. Could you please restart the CI?

Will be scheduled once rc2 is generated (few PRs in CI)

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

starting CI as CI resources are available

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 14, 2019

Test run: SUCCESS

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

@0xc0170 0xc0170 merged commit 5252163 into ARMmbed:master Mar 14, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9248 cycles (-647 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.