Skip to content

DNS send returning WOULD_BLOCK forces delayed retry #9900

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 14, 2019
Merged

DNS send returning WOULD_BLOCK forces delayed retry #9900

merged 1 commit into from
Mar 14, 2019

Conversation

michalpasztamobica
Copy link
Contributor

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
SeppoTakalo suggested changes Mar 1, 2019
View reviewed changes
Copy link
Contributor

@SeppoTakalo SeppoTakalo left a comment

Choose a reason for hiding this comment

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

No printf() should be left there.

@michalpasztamobica
Copy link
Contributor Author

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 a team March 1, 2019 10:00
@ciarmcom
Copy link
Member

ciarmcom commented Mar 1, 2019

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

@VeijoPesonen
Copy link
Contributor

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

@SeppoTakalo
Copy link
Contributor

SeppoTakalo commented Mar 1, 2019 •
edited
Loading

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

@michalpasztamobica
Copy link
Contributor Author

@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
Copy link
Contributor

0xc0170 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
Copy link
Contributor

0xc0170 commented Mar 8, 2019

Reviews please here!

SeppoTakalo
SeppoTakalo reviewed Mar 8, 2019
View reviewed changes
@michalpasztamobica michalpasztamobica force-pushed the dns_async_handle_would_block branch from 0cec313 to 967cd31 Compare March 11, 2019 09:27
VeijoPesonen
VeijoPesonen suggested changes Mar 11, 2019
View reviewed changes
features/netsocket/nsapi_dns.cpp Outdated
@@ -30,6 +30,9 @@
#include "PlatformMutex.h"
#include "SingletonPtr.h"

#define TRACE_GROUP "DNS_API"
Copy link
Contributor

Choose a reason for hiding this comment

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

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");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed actually.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@michalpasztamobica michalpasztamobica force-pushed the dns_async_handle_would_block branch from 967cd31 to ee056da Compare March 11, 2019 11:35
@michalpasztamobica
Copy link
Contributor Author

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

@0xc0170
Copy link
Contributor

0xc0170 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
Copy link
Contributor

starting CI as CI resources are available

@mbed-ci
Copy link

mbed-ci 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xc0170 0xc0170 0xc0170 approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

Assignees
No one assigned
Labels
release-version: 5.12.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@michalpasztamobica @ciarmcom @VeijoPesonen @SeppoTakalo @0xc0170 @NirSonnenschein @mbed-ci @AriParkkila