Stream reuse #283

wcawijngaards · 2020-08-04T09:49:35Z

This implements stream reuse towards Unbound's upstream servers. When a query is sent to a server for which there is an open socket, the socket is reused without opening a new stream to the server. For TCP and TLS queries, for IPv4 and IPv6. Sockets are opened per-thread, since the per-thread worker keeps track of the reused sockets.

This is for upstream pipes and using them again for the next query. Signposted code for reuse_tcp structure in outside_network.h

…am-reuse

…stination find spare id value in reused connection.

fix that comm_point_start_listening does not close the same fd that is started.

fix debug output of reuse tcp and test leak of process

add comm_point indicator for write events for reuse stream writes.

remove pkt args from outnet_tcp_take_into_use, use w.pkt.

Resolved comm_point_start_listening conflict for socket close.

iz8mbw · 2020-11-02T22:09:56Z

Ready to merge? 😁

wcawijngaards · 2020-11-10T10:55:59Z

The branch is ready for use, it is waiting for review(s), before merge.

gthess

Looks good to me also!

I have a question and some nits.
What is the benefit of MAX_REUSE_TCP_QUERIES?

The nits:

Shouldn't log_reuse_tcp(<number>, ...) turn to log_reuse_tcp(<level>, ...) and
verbose(<number>, ...) turn to verbose(<level>, ...)? Unless they are for dev debugging and will be removed?
The ssl_reuse.tdir and the ssl_reuse.* files therein could be renamed to tls_reuse.

services/outside_network.c

services/outside_network.h

testdata/ssl_reuse.tdir/ssl_reuse.conf

testdata/ssl_reuse.tdir/ssl_reuse.test

testdata/tcp_reuse.tdir/tcp_reuse.test

wcawijngaards · 2020-11-23T08:05:02Z

The MAX_REUSE_TCP_QUERIES is so that it does not make too many queries at the same time to a destination. This is because it starts to queue up, but then in terms of TCP buffers, and also, if the connection fails for some reason that is a lot of queries. So I thought it best not to have too many at the same time on one stream, at the level of thousands it should remove overhead costs just fine.

iz8mbw · 2020-11-23T08:25:38Z

Hi. A my personal note, if I have correctly understood, the connection timeout of reuse is hard-coded to 30 seconds.
This means that the ESTHABILISHED TCP connection to forwarder remains ESTHABILISHED up to 30 seconds (if no new DNS query are made within these 30 seconds).
I think 30 seconds is a low value because if no DNS queries are made for 30 seconds the reuse is useless because unbound will go to create new connections to forwarder.
Can be this value increased? I think 60 seconds make more sense.
Thank you.

wcawijngaards · 2020-11-23T09:37:27Z

Sure, increased the reuse time to 60 seconds in 6b97cb1 .

gthess

Thanks for incorporating the feedback, looks good to me!

- Merge PR #283 : Stream reuse. This implements upstream stream reuse for performing several queries over the same TCP or TLS channel.

pengelana · 2020-12-02T13:57:28Z

Unbound 1.12.0 with patch from #283 and --enable-tfo-client & --enable-tfo-server will produce error:

error: read (in tcp s): Transport endpoint is not connected for xxx.xxx.xxx.xxx port 53

dig @127.0.0.1 fedoraproject.org

;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: SERVFAIL, id: 40656
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
;; QUESTION SECTION:
;fedoraproject.org.             IN      A

;; Query time: 213 msec

wcawijngaards · 2020-12-02T14:43:22Z

The fix is in commit 0502ab3 . Thanks for the report!

* nlnet/master: - Fix missing prototypes in the code. Changelog note for NLnetLabs#373 - Merge PR NLnetLabs#373 from fobser: Warning: arithmetic on a pointer to void is a GNU extension. Changelog note for NLnetLabs#335 - Merge PR NLnetLabs#335 from fobser: Sprinkle in some static to prevent missing prototype warnings. Warning: arithmetic on a pointer to void is a GNU extension. - Fix to squelch permission denied and other errors from remote host, they are logged at higher verbosity but not on low verbosity. - Fix NLnetLabs#371: unbound-control timeout when Unbound is not running. - iana portlist updated. - make depend. Code repo continues for 1.13.1 in development. - Fix update, with write event check with streamreuse and fastopen. - Fix for NLnetLabs#283: fix stream reuse and tcp fast open. - Fix on windows to ignore connection failure on UDP, unless verbose. - Fix unbound-dnstap-socket to not use log routine from interrupt handler and not print so frequently when invoked in sequence. - Fix NLnetLabs#356: deadlock when listening tcp. - Fix NLnetLabs#360: for the additionally reported TCP Fast Open makes TCP connections fail, in that case we print a hint that this is happening with the error in the logs. Sprinkle in some static to prevent missing prototype warnings.

wcawijngaards added 30 commits January 16, 2020 17:12

Stream reuse branch, for TCP and TLS stream reuse.

57aefd1

This is for upstream pipes and using them again for the next query. Signposted code for reuse_tcp structure in outside_network.h

Merge branch 'master' into stream-reuse

c79de51

Merge branch 'master' into stream-reuse

3102fa5

Merge branch 'master' into stream-reuse

c9ac6a5

Merge branch 'master' into stream-reuse

af84a27

Merge branch 'master' into stream-reuse

6c14c75

Merge branch 'stream-reuse' of github.com:NLnetLabs/unbound into stre…

014833d

…am-reuse

Merge branch 'master' into stream-reuse

808995e

Merge branch 'master' into stream-reuse

a83f5d7

fixup lru list presence boolean.

1a6cc6e

rename next,prev to lru_next,lru_prev for clarity.

aad363d

tree key addr

d1904bd

add debug printout

d9afcae

add test for tcp reuse

7b46067

find reuse find tcp loop code for multiple connections to the same de…

0f3c638

…stination find spare id value in reused connection.

spare id random selection better.

150e1b0

fix spare id random selection.

7cc6a89

tcp connection is stored and picked up for reuse

fd723ae

fix that comm_point_start_listening does not close the same fd that is started.

fix to set pending pointer in reuse tcp structure

d8b7b5e

fix debug output of reuse tcp and test leak of process

set timeout to wait for reuse

a695ba4

add comm_point indicator for write events for reuse stream writes.

add bool if on tcp waiting list, so that pkt can be stored.

a1babff

remove pkt args from outnet_tcp_take_into_use, use w.pkt.

fix documentation for waiting_tcp pkt NULL setting.

4b6e41e

fix uninit after malloc for on_tcp_waiting_list.

0e0c577

fix crash on cleanup.

d96e718

tcp reuse timeout event cleanup and callbacks.

c809bb9

Fix grammar.

6f93101

fix reuse tcp crash, use addr in reuse struct, free leaked tcp entries.

8ca34be

reuse tcp lookup with correct address as key.

04d805b

Merge branch 'master' into stream-reuse

658e5f1

Merge branch 'master' into stream-reuse

dd096cc

wcawijngaards added 2 commits October 21, 2020 09:54

Merge branch 'master' into stream-reuse

78e9b89

Resolved comm_point_start_listening conflict for socket close.

Merge branch 'master' into stream-reuse

1e9381f

wtoorop requested review from gthess and removed request for wtoorop November 11, 2020 09:53

gthess requested changes Nov 23, 2020

View reviewed changes

wcawijngaards added 9 commits November 23, 2020 09:29

stream reuse, debug output with verbose level instead of number.

d83b197

stream reuse, renamed ssl_reuse.tdir to tls_reuse.tdir.

6ded710

stream reuse, rename ssl_reuse test to tls_reuse test.

fd94b0b

stream reuse, fix review comments.

6f4c79a

stream reuse, review comments.

8143ce6

stream reuse, fix review comments.

4445d9c

stream reuse, fix review comments.

dde9fad

Merge branch 'master' into stream-reuse

9423b5b

stream reuse, up connection reuse time to 60 seconds.

6b97cb1

gthess approved these changes Nov 23, 2020

View reviewed changes

Merge branch 'master' into stream-reuse

ead06af

wcawijngaards merged commit a241136 into master Nov 24, 2020

wcawijngaards added a commit that referenced this pull request Nov 24, 2020

Changelog note for #283.

de4e6ad

- Merge PR #283 : Stream reuse. This implements upstream stream reuse for performing several queries over the same TCP or TLS channel.

wcawijngaards added a commit that referenced this pull request Dec 2, 2020

- Fix for #283: fix stream reuse and tcp fast open.

0502ab3

wcawijngaards deleted the stream-reuse branch December 3, 2020 09:32

cyounkins mentioned this pull request Dec 15, 2021

Have these Stubby features been added to Unbound? MatthewVance/stubby-docker#28

Closed

wcawijngaards restored the stream-reuse branch April 14, 2023 11:21

Stream reuse #283

Stream reuse #283

wcawijngaards commented Aug 4, 2020

iz8mbw commented Nov 2, 2020

wcawijngaards commented Nov 10, 2020

gthess left a comment

wcawijngaards commented Nov 23, 2020

iz8mbw commented Nov 23, 2020 •

edited

wcawijngaards commented Nov 23, 2020

gthess left a comment

pengelana commented Dec 2, 2020

wcawijngaards commented Dec 2, 2020

Stream reuse #283

Stream reuse #283

Conversation

wcawijngaards commented Aug 4, 2020

iz8mbw commented Nov 2, 2020

wcawijngaards commented Nov 10, 2020

gthess left a comment

Choose a reason for hiding this comment

wcawijngaards commented Nov 23, 2020

iz8mbw commented Nov 23, 2020 • edited

wcawijngaards commented Nov 23, 2020

gthess left a comment

Choose a reason for hiding this comment

pengelana commented Dec 2, 2020

wcawijngaards commented Dec 2, 2020

iz8mbw commented Nov 23, 2020 •

edited