For event processing style programming, it's standard to have two event
scheduling "styles" - an immediate "do as soon as possible", and a
"scheduled" style which is guaranteed to unwind the event processing stack
at least once. In another way, it's similar to "call" and "yield" in
co-routines. I will say I'm not sure why TSContSchedule should need a flag
- I'd prefer it always yields, and the new API be the immediate one. Part
of this is to address scw00's concern that a naive plugin writer could
schedule for 0 time in the future, presuming a yield, but in fact creating
an infinite ("dead") loop. Separating the APIs both restores existing
behavior before the 6103 PR and enables providing an API needed to fix a
production problem.On Wed, Nov 20, 2019 at 10:37 AM Sudheer Vinukonda <sudheervinuko...@yahoo.com.invalid> wrote: > Hmm..Why’d the API need to differentiate the implementation details to > users? Alternately, why’d someone pick an API that may have a hole? > > I haven’t fully analyzed and understood the proposed changes, but having > two different API that only differ in how they are implemented under the > hood (and one of them with an apparent hole) doesn’t sound right to me. > > - Sudheer > > > > On Nov 20, 2019, at 8:01 AM, Fei Deng <f...@verizonmedia.com.invalid> > wrote: > > > > Let me rephrase that, the new API behaves the same as the TSContSchedule > > with 0 timeout after PR#6103, which will handle events as soon as > possible. > > While this is good for delays, it also causes the situation scw00 brought > > up (dead loop). And since there is no good way of differentiating this > > behavior with the original one where the event is put into the > > EventQueueExternal, and waiting for the next eventloop (with possible > 60ms > > delay if on the same thread). So I want to change the > > current TSContSchedule code to make sure that we are not suffering from a > > dead loop, and also create a new API such that the new execute ASAP is > > retained. > > > >> On Tue, Nov 19, 2019 at 6:06 PM Leif Hedstrom <zw...@apache.org> wrote: > >> > >> > >> > >>>> On Nov 20, 2019, at 05:52, Fei Deng <duke8...@apache.org> wrote: > >>> > >>> Forgot to mention that this change would restore the old behavior of > >>> TSContSchedule minus the delay and dead loop. > >>> > >>>> On Tue, Nov 19, 2019 at 2:39 PM Fei Deng <duke8...@apache.org> wrote: > >>>> > >>>> While PR#6103 (https://github.com/apache/trafficserver/pull/6103) > >> solves > >>>> the problem of having the 60ms delay (caused by waiting in sleep), it > >> also > >>>> creates an issue where the event loop ends up in a "dead loop" if the > >>>> scheduled event schedules itself with 0 timeout (see test code by > >> scw00). > >>>> Here's what I have in mind that will fix the problem. > >>>> > >>>> > >>>> 1. New API "TSContDispatch", which will be basically the current > >>>> "TSContSchedule" calls with 0 timeouts. > >> > >> > >> Dumb question, why do we need a new API, if it behaves the same as a > call > >> to TSContSchedule with a timeout of 0? > >> > >> I’m getting pretty concerned about these changes, as I think we all > agree, > >> this is critical code, and it’s (as we are seeing) easy to break things > in > >> bad ways. > >> > >> — Leif > >> > >>>> 2. When scheduling events/continuations using "TSContSchedule" API > >> with > >>>> 0 timeouts, set flag to identify the event to be processed on the > next > >>>> event loop. > >>>> 3. New "LocalQueue" class to handle events that are supposed to be > >>>> processed in the next event loop. > >>>> > >>>> Here's the branch I'm working on right now, it shows an easy concept > of > >> the > >>>> "LocalQueue". > >>>> https://github.com/duke8253/trafficserver/tree/master-event_dispatch > >>>> > >> > >> > >
