tensorpipe icon indicating copy to clipboard operation
tensorpipe copied to clipboard

init listener impl boilerplate with runInLoop

Open tscmoo opened this issue 4 years ago • 2 comments

This fixes a bug in core/listener_impl.cc where: context->listen(address) does a deferred init of the listener, but listener->addr()) 2 lines down does a runInLoop addr() on the same listener. When this is run from within the loop itself, init will be deferred but addr will be called immediately. addr will then throw an exception since listener has not been initialized. This fix simply makes init use runInLoop too, which makes it initialize immediately when run from within the loop. Note that the original code usually works since new listeners are usually not created from within the loop, but I don't see why this should not be allowed

tscmoo avatar Feb 10 '21 18:02 tscmoo

I'd like to understand this a bit better. The concrete issue is that creating a listener and immediately trying to access its address while within the loop (in other words, from within a callback?) causes basically an "inversion" of the order of these operations, which then leads to failure due to using uninitialized state. Is that right?

Is this use-case (i.e., creating a listener from within the loop) something you encountered in your application, or is it an "academic" concern? We sort-of assumed that the user (i.e., you) would create the listener and access its addresses in their main thread (at the very beginning of the program) and thus never expected this to happen within the loop.

If we do really want to support this, I'd probably try to find another way, as runInLoop is somewhat of a "hack" to me, and I'd rather reduce its usage than add more of it. The issue with it is that runInLoop can in some cases change the order in which operations are executed compared to how they were scheduled (this is exactly what's causing the bug) which makes it harder to reason about the code and thus to ensure correctness. I could see an exception to that rule for the init method, but I'd still like to explore other options...

lw avatar Feb 11 '21 09:02 lw

Yes, this is an issue I ran into in my application as I lazily initialize some listeners. It's possible I could redesign my code, but this is a fairly simple thing that I would expect to work either way.

tscmoo avatar Feb 11 '21 10:02 tscmoo