it's not a bug, it's how it was supposed to be from the beginning but wasn't functioning correctly.
On Wed, Nov 20, 2019 at 2:27 PM Sudheer Vinukonda <sudheervinuko...@yahoo.com.invalid> wrote: > >>> 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. > > So, isn't this a new bug introduced with PR# 6103? > Either we care about the bug and fix it or if we don't (as you say, _bad > programming_) then document it as a Gotcha/limitation, but, I still don't > see why/how a new API is needed to fix a bug. > > On Wednesday, November 20, 2019, 12:21:00 PM PST, Fei Deng > <f...@verizonmedia.com.invalid> wrote: > > 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 > usingschedule 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 > > >>>> > > >> > > >> > > > >
