application startup should fail if initial download of a single list failed (#310) #313

kwitsch · 2021-10-10T14:58:21Z

Changes:

server.go:NewServer -> returns multpiple errors instead of one
Resolver interface -> new function GetInitErrors returns slice of errors which occured during resolver initialisation
list_cache.go:NewListCache -> additionaly returns slice of errors which occured during resolver initialisation
list_cache.go -> modified error handling

…emporary

kwitsch · 2021-10-11T07:11:03Z

Sorry I'm fairly new to unit tests, will try fixing the problem.

…ors as temporary" This reverts commit b41ccc3.

0xERR0R · 2021-10-11T08:17:31Z

Hi, thank you for your work!
I'll review your PR in detail today or tomorrow. After a short look, I have one question:
You introduced new function in the interface "Resolver": GetInitErrors() []error
Since this function is in the most cases empty (in makes only sense for BlockingResolver), would it not be better to remove this function and return all initialization errors in correspondent "New...Resolver" methods (e.g. NewBlockingResolver)? I think it would be more cleaner, but maybe there is a reason for it...

kwitsch · 2021-10-11T08:49:44Z

Hi, thank you for your work! I'll review your PR in detail today or tomorrow. After a short look, I have one question: You introduced new function in the interface "Resolver": GetInitErrors() []error Since this function is in the most cases empty (in makes only sense for BlockingResolver), would it not be better to remove this function and return all initialization errors in correspondent "New...Resolver" methods (e.g. NewBlockingResolver)? I think it would be more cleaner, but maybe there is a reason for it...

Hi, I added the functions as stub for further error handling.
The upstream_resolver and the parallel_best_resolver could also return errors which are, as far as i know, not currently handeld(upstream resolver is not an IP and cant't be resolved).

I tried to introduce a generic solution for error handling during server startup but I'm still kind of new to the conzept of working without methode overrides and abstract classes so there will be a better solution for sure(got stuck at the Chain function). 😅

0xERR0R · 2021-10-11T09:20:01Z

Its a good idea to prepare the code to handle errors in all resolvers. I think the errors could be handled just before the chain func. following must be replaced

return resolver.Chain(
		resolver.NewIPv6Checker(cfg.DisableIPv6),
		resolver.NewClientNamesResolver(cfg.ClientLookup),
		resolver.NewQueryLoggingResolver(cfg.QueryLog),
		resolver.NewMetricsResolver(cfg.Prometheus),
		resolver.NewCustomDNSResolver(cfg.CustomDNS),
		resolver.NewBlockingResolver(cfg.Blocking),
		resolver.NewCachingResolver(cfg.Caching),
		resolver.NewConditionalUpstreamResolver(cfg.Conditional),
		resolver.NewParallelBestResolver(cfg.Upstream.ExternalResolvers),
	)

by invoking each "New*" function and collecting errors. In error case -> log errors and fail to start, if no error occurred -> call chain function.

This reverts commit 940bf9b.

0xERR0R · 2021-10-11T19:40:29Z

lists/list_cache.go

 	for group, links := range b.groupToLinks {
 		cacheForGroup := b.createCacheForGroup(links)
-


The logic based on "elementCount" doesn't work if a group has multiple links and some of them are successful, for Example this config:

port: 55555 logLevel: info blocking: blackLists: xyz: - /wrongpath/whitelist.txt ads: - http://wrongdomain/hosts.txt - https://s3.amazonaws.com/lists.disconnect.me/simple_tracking.txt upstream: default: - https://8.8.8.8/dns-query - https://8.8.4.4/dns-query

I would expect 2 errors, but I get only one, because the "ads" group has more than 0 elements

I tested this scenario with commit b41ccc3 but as the unit test(list_cache_test.go:133) indicated that this was the expected behavior. The commit was reverted afterwards in commit 98ddbb2.
I missed to test the following solution throughout. 😞

I would propose to let createCacheForGroup additionally return errors, which could be handeld afterwards.

0xERR0R · 2021-10-11T19:43:46Z

lists/list_cache.go

@@ -205,22  219,27 @@ func (b *ListCache) downloadFile(link string) (io.ReadCloser, error) {
 		if resp, err = client.Get(link); err == nil {
 			if resp.StatusCode == http.StatusOK {
 				return resp.Body, nil
+			} else {


0xERR0R · 2021-10-11T20:38:20Z

Hey,
I could take a look on your PR, I understand the idea and your changes in error handling are great. I added some comments, please let us discuss it. I'm open for ideas and I think we are on the good way.

kwitsch · 2021-10-12T08:09:19Z

Hey, I could take a look on your PR, I understand the idea and your changes in error handling are great. I added some comments, please let us discuss it. I'm open for ideas and I think we are on the good way.

Hi, already seen it and answered 😉
I try coming up with a better solution.
For a more unittested solution I need to learn working with unittests in golang first...sorry for the inconvenience 😕

0xERR0R · 2021-10-12T08:15:57Z

Let me know if you need help or if I can take a task or a part

kwitsch · 2021-10-12T11:39:08Z

I tested this solution with your config which results in two errors and 'make test' got through without errors.
No unit test for this scenario jet.

0xERR0R · 2021-10-11T19:48:26Z

lists/list_cache.go

-			attempt  
-		} else {
-			return nil, err
+		} else if errors.As(err, &dnsErr) {


Different message for DNS resolution errors 👍

0xERR0R · 2021-10-11T20:33:29Z

server/server.go

@@ -47,7  47,7 @@ func getServerAddress(addr string) string {
 }

 // NewServer creates new server instance with passed config
-func NewServer(cfg *config.Config) (server *Server, err error) {
+func NewServer(cfg *config.Config) (server *Server, errors []error) {


Discussion: typically in Go, if a function can't handle error internally, it returns an error as the last parameter. It is possible to combine multiple errors into one with wrapping (see error wrapping). Here is some example: https://play.golang.org/p/Kwbek7RV1Lw

I think it would be more "go-ish" to return here just "error" and not a slice of errors. If multiple errors can ocurre, they should be wrapped. This would make the code simpler and convenient.

What do you think?

We can also merge it with error slice, I'm currently not sure what is the better solution

Didn't know about the wrapping and seen error more than some kind of object than an actual exception 😅
Wrapping them seems logical.

codecov · 2021-10-12T19:06:50Z

Codecov Report

Merging #313 (84bd924) into development (c22292e) will decrease coverage by 0.63%.
The diff coverage is 81.44%.

@@               Coverage Diff               @@
##           development     #313       /-   ##
===============================================
- Coverage        95.27%   94.64%   -0.64%     
===============================================
  Files               29       29              
  Lines             2160     2202       42     
===============================================
  Hits              2058     2084       26     
- Misses              74       83        9     
- Partials            28       35        7

Impacted Files	Coverage Δ
resolver/blocking_resolver.go	`98.31% <55.55%> (-1.69%)`	⬇️
util/common.go	`94.06% <61.53%> (-4.09%)`	⬇️
server/server.go	`79.38% <79.16%> (-1.05%)`	⬇️
lists/list_cache.go	`94.28% <92.00%> (-1.80%)`	⬇️
cmd/serve.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c22292e...84bd924. Read the comment docs.

kwitsch added 9 commits October 10, 2021 16:46

FatalOnError accepts multiple errors

e934fba

rework downloadFile error handling

56d818a

removed error check in processFile to include all network errors as t…

b41ccc3

…emporary

rework Refresh function to include init flag and []error response

9273540

NewListCache additionally returns []error

eaab8bd

fix list_cache_test -> ignore additional []error from NewListCache

ef2d6d7

added GetInitErrors to blocking_resolver

940bf9b

added dummy GetInitErrors to other resolver

adc79ec

NewServer could return multiple errors

385b6e9

kwitsch added 2 commits October 11, 2021 09:43

fixed bug in unit test(server_test.go)

21525f8

Revert "removed error check in processFile to include all network err…

98ddbb2

…ors as temporary" This reverts commit b41ccc3.

set cache to nil if count is empty during initialization

241b1e2

kwitsch added 2 commits October 11, 2021 12:47

Revert "added GetInitErrors to blocking_resolver"

5c9581d

This reverts commit 940bf9b.

NewBlockingResolver returns []errors

a9ef915

0xERR0R reviewed Oct 11, 2021

View reviewed changes

kwitsch added 3 commits October 12, 2021 11:21

bugfix in error handling(server_test.go)

90453a0

bugfix TestUDPUpstream(server_test.go)

aebb99d

error handling(list_cache.go)

84bd924

0xERR0R reviewed Oct 12, 2021

View reviewed changes

0xERR0R changed the title ~~Fix for ticket #285 by implementing ticket #310~~ application startup should fail if initial download of a single list failed (#310) Oct 12, 2021

0xERR0R merged commit e5b44f4 into 0xERR0R:development Oct 13, 2021

0xERR0R linked an issue Nov 14, 2021 that may be closed by this pull request

v0.16 crashes at startup if no blacklists are available #286

Closed

kwitsch deleted the #285#310 branch November 15, 2021 10:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

application startup should fail if initial download of a single list failed (#310) #313

application startup should fail if initial download of a single list failed (#310) #313

kwitsch commented Oct 10, 2021

kwitsch commented Oct 11, 2021

0xERR0R commented Oct 11, 2021

kwitsch commented Oct 11, 2021

0xERR0R commented Oct 11, 2021

0xERR0R Oct 11, 2021

kwitsch Oct 12, 2021

0xERR0R Oct 11, 2021

0xERR0R commented Oct 11, 2021

kwitsch commented Oct 12, 2021

0xERR0R commented Oct 12, 2021

kwitsch commented Oct 12, 2021

0xERR0R Oct 11, 2021

0xERR0R Oct 11, 2021 •

edited

Loading

kwitsch Oct 12, 2021

codecov bot commented Oct 12, 2021

		for group, links := range b.groupToLinks {
		cacheForGroup := b.createCacheForGroup(links)

application startup should fail if initial download of a single list failed (#310) #313

application startup should fail if initial download of a single list failed (#310) #313

Conversation

kwitsch commented Oct 10, 2021

kwitsch commented Oct 11, 2021

0xERR0R commented Oct 11, 2021

kwitsch commented Oct 11, 2021

0xERR0R commented Oct 11, 2021

0xERR0R Oct 11, 2021

Choose a reason for hiding this comment

kwitsch Oct 12, 2021

Choose a reason for hiding this comment

0xERR0R Oct 11, 2021

Choose a reason for hiding this comment

0xERR0R commented Oct 11, 2021

kwitsch commented Oct 12, 2021

0xERR0R commented Oct 12, 2021

kwitsch commented Oct 12, 2021

0xERR0R Oct 11, 2021

Choose a reason for hiding this comment

0xERR0R Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

kwitsch Oct 12, 2021

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2021

Codecov Report

0xERR0R Oct 11, 2021 •

edited

Loading