Skip to content
This repository has been archived by the owner on Jul 1, 2023. It is now read-only.

init listener impl boilerplate with runInLoop #295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tscmoo
Copy link

@tscmoo tscmoo commented Feb 10, 2021

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

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 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
@lw
Copy link
Contributor

lw commented Feb 11, 2021

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...

@tscmoo
Copy link
Author

tscmoo commented Feb 11, 2021

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants