It's not a hole, it's how it should have been all along, but have not been working properly for a long time. schedule_imm is supposed to put things in queue and have the eventloop process them asap and not waiting for IO or other stuff, but instead it stays in queue till we wake up the thread either by signal from others, or waiting out the sleep time. With PR#6103, that's been addressed and also fixes the problem with the unwanted delays, but as scw00 pointed out, that will cause an issue for devs that are using schedule with 0 timeouts in their code to schedule itself over and over again (see test code in PR comment), which is bad programming on their part but it might happen and is a valid concern. With the changes proposed here, the new API will handle that extreme use case where you want things to run asap without any kind of waiting, and make TSContSchedule calls with 0 timeouts behave like it used to be (even though it isn't what it should be), which is put the event into a queue and wait for the next eventloop to handle them. And with the new changes I mocked up in the first email, it also addresses the part where wait time can only be efficiently calculated using the priority queue (EventQueue), but not the external queue (EventQueueExternal).
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 > >>>> > >> > >> > >
