0

I currently working on fixing bug in the following method which polls stateChecker condition while it null until it becomes true (or false due to timeout):

private static void WaitWithSubject(
   Func<bool> stateChecker,
   TimeSpan timeout,
   TimeSpan stepTime, 
   string errorMessage,
   ILifetimeInfo lifetimeInfo)
{
   (bool? IsOk, string Message) state = (IsOk: null, Message: string.Empty);
   var waitCancellation = (int)stepTime.TotalMilliseconds;
   using (var stateSubject = new Subject<(bool? IsOk, string Message)>())
   {
      using (Observable.Timer(timeout).Subscribe(it => stateSubject.OnNext((IsOk: false, Message: errorMessage))))
      using (Observable.Timer(TimeSpan.Zero, stepTime).
         Subscribe(it =>
         {
            if (stateChecker())
               stateSubject.OnNext((IsOk: true, Message: string.Empty));
         }))
      {
         using (stateSubject.Subscribe(it => state = it))
         {
            while (state.IsOk == null)
               lifetimeInfo.Canceler.ThrowIfCancellationRequested(waitCancellation);
            if (state.IsOk != true)
               throw new TimeoutException(state.Message);
            stateSubject.OnCompleted();
         }
      }
   }
}

This method occasionally generates ObjectDisposedException at following point in code on executing method OnNext :

if ( stateChecker() )
    stateSubject.OnNext( ( IsOk: true, Message: string.Empty ) );

Is there a way of totally avoid using Subject in this case in favor of something like Observable.Interval or Observable.Create?

LukeS5310
  • 3
  • 1
  • 4
  • There are a couple of issues here, but maybe an RX expert can refactor this for you in some degree. Paging @Enigmativity – TheGeneral Mar 11 '21 at 14:10
  • Regarding this line: `while (state.IsOk == null) lifetimeInfo.Canceler.ThrowIfCancellationRequested(waitCancellation);`, could you edit the question and post the `ThrowIfCancellationRequested` method? – Theodor Zoulias Mar 11 '21 at 14:29
  • 1
    Also could you provide a brief explanation of what this method is intended to do? – Theodor Zoulias Mar 11 '21 at 14:51
  • So this method blocks the current thread while the `stateChecker()` returns `false`, returns normally once the `stateChecker()` returns `true`, and returns exceptionally (`TimeoutException`) in case the timeout period has been surpassed, correct? – Theodor Zoulias Mar 11 '21 at 15:35
  • Yes, almost. It blocks execution by given amount of time while `isOk` is `null` – LukeS5310 Mar 11 '21 at 15:50
  • 1
    @TheGeneral - I've put up an answer. The big problem with the code in the question is the nested subscriptions - that should never happen. – Enigmativity Mar 14 '21 at 09:20

2 Answers2

2

It seems to me that this is what you're trying to do:

private static void WaitWithSubject(Func<bool> stateChecker, TimeSpan timeout, TimeSpan stepTime, string errorMessage, ILifetimeInfo lifetimeInfo) =>
    Observable
        .Amb(
            Observable
                .Timer(timeout)
                .SelectMany(_ => Observable.Throw<Unit>(new TimeoutException(errorMessage))),
            Observable
                .Timer(TimeSpan.Zero, stepTime)
                .Where(_ => stateChecker())
                .Select(_ => Unit.Default))
        .Take(1)
        .Wait();

The key here is the Amb operator which starts the two sequences and only returns values from the first one to produce a value or an error. The Take(1) ensures that the observable finishes as soon as a value is produced.

You can throw the following line in just before the Wait() to cancel if you have a CancellationToken:

.TakeUntil(Observable.Create<Unit>(o => ct.Register(() => o.OnNext(Unit.Default))))

After a bit of back and forth with Theodor, I've come up with this version that I think is possibility the cleanest I can think of:

private static void WaitWithSubject(Func<bool> stateChecker, TimeSpan timeout, TimeSpan stepTime, string errorMessage, ILifetimeInfo lifetimeInfo)
{
    var good =
        Observable
            .Timer(TimeSpan.Zero, stepTime)
            .Where(_ => stateChecker())
            .Take(1);

    var fail =
        Observable
            .Timer(timeout)
            .SelectMany(_ => Observable.Throw<long>(new TimeoutException(errorMessage)));
    
    good.Merge(fail).RunAsync(lifetimeInfo.Canceler).Wait();
}
Enigmativity
  • 113,464
  • 11
  • 89
  • 172
0

Here is a method that has similar behavior with the WaitWithSubject, without using a Subject<T>. It uses the Merge operator, in order to combine two timer-generated sequences into one sequence. It also features cancellation support.

public static void WaitUntilTrueState(
    Func<bool> stateChecker,
    TimeSpan checkInterval,
    TimeSpan timeout,
    string timeoutMessage,
    CancellationToken cancellationToken = default)
{
    Observable
        .Timer(TimeSpan.Zero, checkInterval)
        .Merge(Observable.Timer(timeout).IgnoreElements()
            .Concat(Observable.Throw<long>(new TimeoutException(timeoutMessage))))
        .TakeUntil(_ => stateChecker())
        .RunAsync(cancellationToken)
        .Wait();
}
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • 1
    Thank you. I'll adapt it to my code and let you know the results – LukeS5310 Mar 12 '21 at 05:28
  • Works like a charm, thank you again. I posted the adapted verision in my question. – LukeS5310 Mar 12 '21 at 08:58
  • @LukeS5310 I have some doubts about your implementation, mainly because I have no idea what the `Canceler.ThrowIfCancellationRequested` method is doing. I hope that it doesn't block the current thread, because that would interfere with the timeout scheduling. – Theodor Zoulias Mar 12 '21 at 12:27
  • Actually it does block the current thread. Should I place it sowhere else in my code? Unforntunately I can't avoid using this method altogether – LukeS5310 Mar 12 '21 at 12:36
  • Assuming that it blocks the thread for 1000 msec, then in case the `stepTime` time span is longer than this, the "ticks" generated by the `Observable.Timer` are going to be accumulated, because they will be produced faster than they will be consumed. This would result to increased memory consumption. This may or may not be a problem, depending on a number of parameters, but it is certainly "smelly". I can't offer more specific advices without seeing that method, and seeing how the `PollState` method is used in practice. – Theodor Zoulias Mar 12 '21 at 12:47
  • Done some research, method `ThrowIfCancellationRequested` has an overload which is not blocking thread, just checking whether cancellation flag is raised, updating code accordingly – LukeS5310 Mar 12 '21 at 12:59
  • @LukeS5310 I updated my answer with an improved implementation. The supplied `cancellationToken` is not just checked periodically. It is registered with the observable, and in case of cancellation the termination of the `Wait`ing will be instantaneous. – Theodor Zoulias Mar 14 '21 at 13:18
  • The `.TakeUntil(_ => stateChecker())` looks wrong. It's going to produce a series of `false` rather than a single `true` that the OP's code had. – Enigmativity Mar 14 '21 at 21:53
  • @Enigmativity the OP has not made the requirements crystal clear. My assumption is that the method should block the current thread while the `stateChecker()` returns `false`, and should return once the `stateChecker()` returns `true`. The name I gave to the method (`WaitUntilTrueState`) reflects this assumption. – Theodor Zoulias Mar 14 '21 at 22:24
  • @TheodorZoulias - My mistake on the `TakeUntil` - it returns a final value when the predicate returns `true`. I thought it didn't return the final value. Combined with the `RunAsync` that then works fine. It's not a super clear set of operators, imho. – Enigmativity Mar 15 '21 at 00:23
  • @Enigmativity yeap, I think that's the subtle difference between `TakeUntil` and `TakeWhile`: whether the last element is included or not. For this particular usage probably any of these operators could be used, without making much of a difference. – Theodor Zoulias Mar 15 '21 at 01:59