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

Feature/configure outbound msg retry #41

Conversation

countsudoku
Copy link
Contributor

@countsudoku countsudoku commented Jun 12, 2019

I've added a option to configure the number of retries unbound will make in case of a THROWAWAY answer. In case of forward nameservers, this will restrict the number of retries for every nameserver (more precise for every target).

I try to use libunbound in a high performance application. Therefore it is important to restrict the number of request unbound will make.

@countsudoku countsudoku reopened this Jun 12, 2019
@countsudoku countsudoku changed the title :qaFeature/configure outbound msg retry Feature/configure outbound msg retry Jun 12, 2019
@wcawijngaards
Copy link
Member

Hi Countsudoku,
This patch looks very good. Currently in the process of fixing up bugs in the release so I do not want to introduce features (possible failures could be in it), right now. So I won't accept the PR straight away, and leave it here for a bit to keep it separate from the release concerns. Thanks for the excellent code!

@countsudoku
Copy link
Contributor Author

Currently in the process of fixing up bugs in the release so I do not want to introduce features (possible failures could be in it), right now. So I won't accept the PR straight away, and leave it here for a bit to keep it separate from the release concerns.

Thanks for the info. You can also pick only some commits, since I've committed also some style fixes etc. I am not sure, if it would be better to re-generate the autogenerated files (util/configlexer.c, util/configparser.{h,c}) again with your flex version. Therefor I have committed these files in a separate commit. I am curious: Would it not be better to keep these autogenertaed files out of the repo and create them on demand?

@countsudoku
Copy link
Contributor Author

Is there any chance that this will be merged in the near future?

@boarfish55
Copy link

Would love to see this merged as well. In the organization I work in, we sometimes have multiple unbound resolvers "chained" (one running on the client, then another one caching in front of the authorities), and the fact that they each retry 5 times by default on a THROWAWAY response means the latency at the client increases exponentially.

If we have a lame server on our network, or elsewhere, this means the client can sometimes have to wait 1+ second(s) before getting a reply, which in some cases causes timeouts. We would love to drop this value to one on "intermediate" resolvers and leave it to the client to make the decision as to retry to not.

Copy link
Member

@wcawijngaards wcawijngaards left a comment

Choose a reason for hiding this comment

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

The code looks good; have some smaller fixes I want to do. A lot of whitespace changes (of existing code), that is nicer in separate patches; diff that ignores whitespace can very quickly deal with the review of it. But it is okay here. I'll try to commit and fix the conflicts.

@wcawijngaards wcawijngaards merged commit 204edd2 into NLnetLabs:master Sep 8, 2021
wcawijngaards added a commit that referenced this pull request Sep 8, 2021
  processQueryResponse takes an iterator env argument like other
  functions in the iterator, no colon in string for set_option,
  and some whitespace style, to make it similar to the rest.
@wcawijngaards
Copy link
Member

Thanks for the patch! Merged it in the code repository.

jedisct1 added a commit to jedisct1/unbound that referenced this pull request Oct 5, 2021
* nlnet/master: (118 commits)
  - Fix to add example.conf note for outbound-msg-retry.
  - Implement RFC8375: Special-Use Domain 'home.arpa.'.
  - Fix crosscompile script for the shared build flags.
  - Fix crosscompile windows to use libssp when it exists. - For the windows compile script disable gost. - Fix that on windows, use BIO_set_callback_ex instead of deprecated
  - Fix crosscompile shell syntax.
  - For crosscompile on windows, detect 64bit stackprotector library.
  - Fix crosscompile on windows to work with openssl 3.0.0 the   link with ws2_32 needs -l:libssp.a for __strcpy_chk.   Also copy results from lib64 directory if needed.
  - Fix more initialisation errors reported by gcc sanitizer.
  - Fix lock debug code for gcc sanitizer reports.
  - Fix initialisation errors reported by gcc sanitizer.
  - Fix root_anchor test to check with new icannbundle date.
  - Fix for NLnetLabs#41: change outbound retry to int to fix signed comparison   warnings.
  - Small fixes for NLnetLabs#41: changelog, conflicts resolved,   processQueryResponse takes an iterator env argument like other   functions in the iterator, no colon in string for set_option,   and some whitespace style, to make it similar to the rest.
  Changelog entry for NLnetLabs#538 - Fix NLnetLabs#538: Fix subnetcache statistics.
  Fix subnetcache statistics
  - Fix tcp fastopen failure when disabled, try normal connect instead.
  - Fix NLnetLabs#533: Negative responses get cached even when setting   cache-max-negative-ttl: 1
  - Fix asynclook unit test for setup of lockchecks before log.
  - Fix compile warning in libunbound for listen desetup routine.
  - Fix RPZ locks. Do not unlock zones lock if requested and rpz find   zone does not find the zone. Readlock the clientip that is found   for ipbased triggers. Unlock the nsdname zone lock when done.   Unlock zone and ip in rpz nsip and nsdname callback. Unlock   authzone and localzone if clientip found in rpz worker call.
  ...
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