if in main thread, management server already sent back response with the alias, do you want to send another on demand request to management server?

Depends on management server implementation, if it only sends a response when there is some update of the project, then this HttpRequest will hang till timeout, while Envoy has the config to proceed.

This gets pretty complicated quickly: the only time (that I can see) an instance of RdsRouteConfigProviderImplcan be deallocated is when LDS is used and the listener configuration has changed. At this point all connections are going to be drained, trying to update a vhost seems not worth it to me.

Even if we wanted to do that, we'd still need an active subscription (may not be there yet, and no way to find out which one to use) and a place to store the callback to notify the on_demand_update filter (which may not be there any more, as the active stream it belongs to may have been closed).

I am describing a scenario where no rds subscription destruction is involved.

could the management server send required vhost to Envoy before the on-demand request is sent? in that case, the management server may or may not send the requested resource again depends on its implementation, in which case the callback will not be called and the request will only timeout. this is very implementation specific, so I am not too concerned.

you can move this into the if statement below.

could you also move this sub lock to the if statement below?

why capture a weak ptr if you dont ever validate that its still alive? maybe just capture this as a weak ptr?

Why don't we allocate the provider itself with shared_from_this and then we can capture a weak reference to ourselves and post that? This seems a bit strange and fragile?

I'd hesitate to do so, with that we could get into another rabbit hole where a worker thread may accidentally get a shared_ptr of the provider and introduce another bunch of life cycle problem due to provider can only be con/destructed on main thread.
thoughts?

OK fair enough. Can we add a comment on why we are not doing that so the next person that comes along doesn't ask the same question? Thank you.

/wait

Ahh, yes, this is better.

can we please not do this. When provider is capable of shared_from_this, we actual open its access to worker threads, which I'd hope we can avoid: providers are supposed to be created/owned by HCM and no one else should have ownership to it, otherwise it'd introduce lots of troubles due to destruction order when Envoy tears down.
If you need a cleaner signal to decide if we need a post, one thing you can do is to lock on route_config_updated_cb, which the finest grained object that tells if the source httprequest is alive or not.

I don't quite understand this now that I think about it more. This is already a shared_ptr allocated object. Why does it matter if it enabled shared_from_this?

I think we want it shared between listeners in main thread, but its ownership should not be shared to worker thread.
If however the ownership of the provider is shared to worker thread, the scenario i was talking about would cause the assertion error when LDS deletes the listener or when envoy quits.

I think the change is quite safe: we aren't changing provider interface, just the concrete implementation: to create another shared_ptr one would need to cast a RouteConfigProvider object to RdsRouteConfigProviderImpl before being able to do so. Code review would catch such a cast or a call to shared_from_this(). I can add a note re: intended usage of shared_from_this if that would help.

I agree that this is the right change modulo any other lifetime issue.

it's a tricky case and it's not about life cycle of provider when the post lambda gets called.
the problem is that The ownership of any "tls instance owner" should not be spilled into other threads than the main thread.
please see my other reply why I am against this, your change is introducing another race which could ends in an assertion failure.

this could cause trouble as it introduces another race between worker threads and main thread: for example on envoy teardown, the lambda created in the worker thread may be the last one to be teardown, in which case, the destruction of the provider will happen in worker thread, which triggers an assert failure because it holds an TLS instance which can only be destructed in main thread.

But how is this safe then? We capture "this" so it can still crash?

i am thinking capturing a member of the provider will guarantee that the provider is alive when this cb is called inside main thread.
The difference is the ownership of provider has not been "shared" outside of main thread, which will not introduce the new race.

i am thinking capturing a member of the provider will guarantee that the provider is alive when this cb is called inside main thread.

It definitely does not guarantee that. It just prevents the member from going away.

let me rephrase why this change is a regression:

1. this lambda "L" is created with a shared pointer to the RDS config provider, and it's in worker thread.
2. it's destructed in worker thread after it's posted to main thread,
3. in main thread, the rds config provider could go away because of LDS or Envoy teardown, in which case the ownership of the config provider will fall to the "L" if it's not deleted from the worker thread yet.
4. deallocation of the RDS config provider in worker thread will cause an assertion failure, which is our fence on clear boundary between config pipeline and data plane, as the tls instance held by the provider can only be destructed in main thread.

Yes I agree ^ is a problem, but this code is still broken. If the provider goes away on the main thread, we still capture this. That is not correct. I think given how long we are talking about this we probably need a cleaner solution here. If the original change is actually safe can the comments be clearly updated to why?

Right. So if we capture a weak_ptr of the callback list, and verify it's alive in main thread, we can make sure that we are accessing an active provider(and its subscription), meanwhile, the provider ownership is still on main thread and we dont need to worry about the race mentioned above.

trying to lock on the route_config_updated_cb will also do the trick, but it has a different semantic: if the callback is alive, which means the hcm is alive and the provider associated with it will be alive.
but now we are firing an on-demand when the requesting HttpRequest is alive, if we lock on provider, we are firing as long as there was a request(may be gone).

Yes, OK, I think this make sense. @dmitri-d can we revert back to what you had before but also add a lot more comments? I would like to get this merged as I'm dealing with additional issues in this area and I want to fix on top of this depending on if anything else needs fixing. Thank you!

/wait

Do we need to capture a weak_ptr to both subscription and callback list? Or would just subscription work? I think the original solution looks right, but it seems awkward to have to lock two objects. Better documentation on lifetime guarantees and relationships would definitely help. I think what you really want is some sentinel member element that provides "am I alive?"; normally this could be shared_from_this but this doesn't work for the reasons @stevenzzzz points out.