[GENIUS-122] DataTreeEventCallbackRegistrar really needs a timeout Created: 12/Apr/18  Updated: 29/May/18  Resolved: 28/May/18

Status: Resolved
Project: genius
Component/s: None
Affects Version/s: None
Fix Version/s: Oxygen-SR2, Fluorine

Type: New Feature Priority: Medium
Reporter: Josh Hershberg Assignee: Michael Vorburger
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Blocks
blocks GENIUS-119 Auto tunnels: race between creation o... Verified

 Description   

Now that the registrar is being used in more general code flows there really needs to be a safety mechanism in the form of a timeout just in case the specific expected event never happens.



 Comments   
Comment by Josh Hershberg [ 12/Apr/18 ]

Here's my design idea. I think there should be new versions of the onX methods that accept two more params: 

onX (..., long timeout, Runnable onTimeout) ...Runnable is what you use if you want to pass a lambda that takes no params and returns nothing, right?

I think we should consider deprecating the old versions eventually. 

 

Comment by Josh Hershberg [ 12/Apr/18 ]

Actually, scratch that. It should take a Consumer<InstanceIdentifier> which should be the instance identifier the registration was on so that if there were two registrations from the same code for different objects in the md-sal they can be differentiated by the timeout routine.

Comment by Michael Vorburger [ 16/Apr/18 ]

That makes sense to me, with a Consumer<InstanceIdentifier<T>> ... I'm a little undecided how we'll related this to NextAction UNREGISTER VS CALL_AGAIN - does a timeout make sense for both? Let me see if I can put together a Gerrit with a (first version of) this, and we we can review the concrete code.

Comment by Michael Vorburger [ 16/Apr/18 ]

actually it really should be a  Consumer<DataTreeIdentifier<T>> not a  Consumer<InstanceIdentifier<T>> 

Comment by Michael Vorburger [ 16/Apr/18 ]

c/70985 has (an initial first version of) this now.. still Draft; let the build situation get sorted, and I'll undraft it, for build. If that gets +1, I'll need to still extend it to cover all methods, and perhaps somehow take this idea from jhershbe into account in a follow-up change improvement:

I think that you need to make sure that the timeout call can not happen while the handler function is running. Like, if a notification happens and just then the timer fires, the timeout event should wait until after the DTCL finishes calling the supplied handler function.

Comment by Michael Vorburger [ 16/Apr/18 ]

vorburger getting the concurrency correct for this is actually not trivial; further to above, what if e.g. the DTCL were to fire while we've just called the timeout callback (but may not yet be finished with it), that's not yet handled (as of Change Set 2), and probably more such cases.

tpantelis may help us getting this right and if time will follow-up on c/70985 with edge case tests and needed fixes.

Comment by Michael Vorburger [ 28/May/18 ]

Done now. k.faseela has also back-ported this from master (Fluorine) to stable Oxygen AFAIK.

Generated at Wed Feb 07 19:59:58 UTC 2024 using Jira 8.20.10#820010-sha1:ace47f9899e9ee25d7157d59aa17ab06aee30d3d.