@tripleee That pattern looks like it's already caught by Potentially bad keyword in answer and Potentially bad keyword in body Append -force to the command if you really want to add this pattern.
@TylerH ╳ You are not a privileged user. Please see the privileges wiki page for information on what privileges are and what is expected of privileged users.
@ThomasWard No, I don't know off the top of my head. I'd have to look at the code. ... It looks like we implement a separate cache for DNS results in findspam.py, which is pruned to a maximum of 1000 entries every time there's a cache miss and there are > 1500 entries. Given that it's in findspam.py, it should be cleared each time SD reboots, but not each time the blacklists are updated. Effectively, this means that the cache should last no longer than 6 hours, due to the 6 hour periodic reboot.
Obviously, there would also be any DNS caching which is implemented at the system level.
A brief search indicates that the dnspython package which we use implements a couple of different types of caches, which are disabled by default, and we do not enable either of them. One of those is a least recently used based cache, which has the ability to specify the maximum number of entries in the cache. I note that the cache code which we have is not thread-safe, but the ones which are implemented by dnspython are.
I assume we use our own cache implementation for one of a variety of possible reasons, but I don't know the specific reasons for that choice. Given the thread safe/not thread safe issue, I'm tempted to switch over to using the LRU cache which is implemented in the dnspython package (rather than working to make ours thread safe). Doing so would have the side effect of also using the cache in chatcommands.py, blacklists.py, and the dnsresolver.py class.
@ThomasWard OK. Is there a reason not to just use one of the the already implemented caches in the dnspython package? If our cache is moved out of findspam.py, then there's more of a need to make it thread safe. Even in findspam.py, if we continue to use it, then it should be made thread safe.
@Makyen My goal is to allow us to disable the in-memory cache which is a dict right now
and rely on the system resolver or the DNS Resolver directly for system cached results
(which is what Art thought it did already)
@Makyen My 2 cents is "Rely on system resolver cache" or "Rely on Nameserver resolver cache if using a non-system resolver". Apparently I accidentally introduced some kind of circular dependency so might have to stab something
@ThomasWard My 2 cents would be: use one of the caches implemented in the dnspython package, or don't have one separately implemented in SD, unless we're getting some significant performance improvement by using a cache we're implementing ourselves, which could primarily be that ours is not thread safe (which would introduce some overhead, but is needed).
I suspect ours was implemented either because the person who did so wasn't aware of the ones in the dnspython package, or that they did not exist at the time ours was implemented.
Basically, I'm not seeing what benefit we're getting from implementing our own which isn't outweighed by it not being thread safe.
@Makyen if we do anything, I'd like to extend our DNSResolver class and add a local DNSResolver.cache object that can either be initialized or not based on a config argument (dns.resolver.Cache)
customizable-dns initiates a classes.dnsresolver.DNSResolver object on load that can be referred to everywhere
for the dig command, etc.
but that's after getting an enable/disable function
and i'd rather use that because it's a timed dump, data is only cached up to cleaning_interval
which means if we have an entry it's flushed automatically
but first I want to get the DNS customizable before I start deep diving into caching.
@ThomasWard Unless the the dnspython package is doing something special, just importing the package should result in a single resolver being used by all the packages which import it within the same overall executable. That's one of the reasons that the cache needs to be thread-safe.
@Makyen There's no way to set the 'cache' on a dnspython object except at init time with our extended __init__ (classes.dnsresolver.DNSResolver), or modifying it before use, i.e. init the resolver object and then do resolverobj.cache = dns.resolver.Cache
with our DNSResolver class I can init that at runtime
and that's threadsafe because DNSResolver is an extended class of dns.resolver.Resolver() to allow additional init options
if we want the cache to be 'global' we use a globalvars Resolver object
@TylerH I meant I could report it for you instead. But Smokey will take it in SOCVR as well (there just won't be any response because it's limited to reporting SO in there)
@Makyen i don't think the issue is 'thread safe caching', but more 'how do we tell the DNS Resolver() instance to use a cache object. Which, we can do at init time, and still be thread safe
the only difference with DNSResolver vs. a straight dns.resolve.Resolver is that we can do some initial configuration automatically at start time, and it'll still be a threadsafe process
@ThomasWard the parts I touched had a really aggressive prune strategy, it just basically tries to avoid asking DNS again while handling the same post ... I was looking at that and thinking it would probably be useful to make the cache bigger, but I never did anything about it
@tripleee there's a few ways to do it, but as Makyen suggested, we should use a thread-safe version that's implemented in the DNS Resolver we query
rather than sustaining an in-memory dict for it (which isn't necessarily thread safe, and could permit us to drop a global declared DNS_CACHE variable)
@ThomasWard It appears there's only ever one resolver in use. None of the calls we make appear to instantiate a new resolver object from the dnspython package (i.e. all use the single base resolver). While turning on one of the caches provided in the dnspython package is desirable to do at init time, it appears that it can be done at any time. In other words, it appears that the setup for the resolver could be in ws.py and affect everything which subsequently imports dnspython.
@ThomasWard Actually in use within SD which both use the the dnspython package, or just as possibilities to select to config the one which is used by the dnspython package?
(1) Initalize a globally referrable DNS Resolver object, that is configured based off config options (nameservers or 'system', cache enabled/disabled, cache TTL)
(2) use that resolver for any resolutions that we do
currently because it sits in chatcommands for dig and findspam, we need to initialize both and use Caching if we enable it
which also, incidentally, fixes the need for us to have keep our own cache because we'll let the resolver object have its own cache initialized or not at initialization of the Resolver object
@Makyen The possibilities in my case are to configure the resolver used by SmokeDetector, which all is going to be a dns.resolver.Resolver class (dns.* is dnspython when I talk to it outside of GlobalVars.dns which is a Resolver object.)
the problem with init of a Resolver is that you can only do pretty much no real configuration
no ability to add Nameservers manually unless you do it after object init, etc.
Basically, DNSResolver in classes.dnsresolver is identical to dns.resolver.Resolver but lets us do the initialization of important items without having to physically reinitialize or reinstantiate configuration options each time you refer to dns.resolver.Resolver() objects.
@Ollie That pattern looks like it's already caught by Potentially bad keyword in answer and Potentially bad keyword in body Append -force to the command if you really want to add this pattern.
I'm not seeing anything that's actually giving a separate instantiation to the the dnspython package's resolver (i.e. the ability to have more than one resolver in use at the same time). As far as I'm seeing, all of those are the same resolver.
It could be I'm not understanding what is being provided by the the dnspython package, but that's what it looks like, and in the still brief looks I've taken at the documentation and the existing code, there isn't anything which indicates there are separate instantiations of the the dnspython package resolver which are separately configured within the same overall executable.
@Makyen ^^ this is basically just initializing the superclass (dns.resolver.Resolver) and taking the three lines to configure rDNS and use it into an initialize-and-use two lines
because otherwise we have to do all those individual lines manually at initialization time
OTHERWISE it's still dnspython's dns.resolver.Resolver() class, it's just a simplified way to configure it at instantiation time without needing to know the internals
classes.dnsresolver also implements resolve_dns for the dig command
@cigien It's strictly off topic according to their help/on-topic ("Operation of..." on diy.stackexchange.com/help/on-topic), but I have a hard time justifying tp
@Undo Yeah, looking at the profile, and the way the question is framed, along with the link, I can't see how that's not TP. If that's not spam, it looks very close to it. Is there some particular reason you're not comfortable with TP on that?
@Makyen the 'sysdns' example is the 'default' for resolve_dns (a function not a class) for the dig function
resolve_dns actually refers to a new DNS Resolver instance like sysdns does in that example but during its creation as the 'default' object - which is recreated every time someone calls !!/dig which was the DNS verification thing
because our DNS cache said one thing but actual DNS said another (refer to @ArtOfCode's hastily written 'dig' wrapper)
but because we force cache anyways in the current code, we don't have the ability to 'clean' that in a thread safe way until the pruner eventually prunes it
@Makyen by using our instantiation with DNSResolver, we can configure a cache built-in at creation time (using dns.resolver.Cache which implements a TTL for any given cached record, like a normal DNS server does), and then that cache is global if the Resolver() or DNSResolver() object is also globally available
@ThomasWard So... just changing to using one of the caches in the dnspython package, which both obey DNS TTL, and can be either pruned on a schedule or have a max size.
@ThomasWard My impression is that dns.resolver is persistent and that setting a cache for that would be persistent for the entire program, unless you then instantiate an additional dns.resolver.Resolver(), which could then have separate settings.
creates a brand new Resolver instance without caching every single time we don't have an item in DNS_CACHE which is just a dict we keep in memory at run time
we COULD create a DNS_CACHE = dns.resolver.Cache object but then we have to do resolver.cache = DNS_CACHE
