Open njsmith opened 6 years ago
I don't see any problem with simply exposing them as public; it also means easy resetting is possible (by saving the token, doing the calls, and restoring it later) whereas the tokens are silently discarded with the set_() functions (at least currently).
This is a pretty low-priority issue, but something to think about before 1.0 anyway:
Once #478 is merged, we'll have a few
RunVar
objects used directly in trio itself: the default thread limiter exposed viatrio.current_default_worker_thread_limiter
, and the hostname resolver and socket factory test hooks exposed viatrio.socket.set_custom_hostname_resolver
andtrio.socket.set_custom_socket_factory
.For the last two in particular, the public functions are basically the simplest possible set/get APIs; they exist solely to hide the clunky old
RunLocal
API. Now we haveRunVar
, which is ... basically the same thing, a more usable API for setting/getting these kinds of variables. So maybe we should just... expose that? So e.g.set_custom_hostname_resolver
would becomecustom_hostname_resolver.set
?The worker thread limiter is a little more subtle. Right now we don't expose any way to change the underlying default limiter object itself; you can adjust the number of simultaneous threads, by getting the object and mutating its
total_tokens
attribute, but you can't replace it wholesale with some other policy. Is there any use case for other policies? I guess you could make a C# thread pool style policy that admits threads after a delay that's proportional to the current pool size... probably anything fancy like this would be better handled by overriding the limiter for specific calls torun_sync_in_worker_thread
, though. OTOH, exposing aRunVar
isn't particularly problematic either. (I guess it would let you override the default with one that's just broken, but I don't think there is any particular danger here beyond any other sort of broken code.)