0

I am trying to refactor some WPF code and found a strange code sequence. Can you help me to understand its implications?

I am in a custom VirtualizingPanel. Inside MeasureOverride a DispatcherTimer is used to defer the call that virtualizes all items that are not longer shown (see here why this is possible).

Using a DispatcherTimer to defer some call on the UI thread seems like a strange choice to me. This is the first thing that I do not understand but considering the rest of the code, the developer might have chosen it because he was familiar with this construct. I would like to change this to a simple Dispatcher.BeginInvoke-call.

Anyway, the second strange thing is the locking that is applied. I assume it is done to secure access to instance fields that are shared between the code inside MeasureOverride and the CleanUpItems lock. But this makes no sense to me, because everything is executed on the UI thread anyway, right?

If this is true, this code might actually introduce a deadlock in case the dispatcher stops one execution inside a lock and continues with another task on the dispatcher queue that requires the lock, right? Or does WPF guarantee that tasks on the the dispatcher queue are not interrupted for other tasks later in the queue?

All this thoughts might be irrelevant, if the the reason for the lock is something else. So if you know another reason that could make sence, please share it with me. In the end I would like to get rid of this lock without any negative consequences.

PS: The ScrollCanvas is created in XAML.

public class ScrollCanvas : VirtualizingPanel, IScrollInfo 
{
    private readonly DispatcherTimer _idleTimer;

    public ScrollCanvas()
    {           
        _idleTimer = new DispatcherTimer(TimeSpan.FromMilliseconds(5), DispatcherPriority.Normal, CleanUpItems, Dispatcher.CurrentDispatcher);
        _idleTimer.Stop();
    }

    protected override Size MeasureOverride(Size availableSize)
    {
       lock (_idleTimer)
       {
            //Finding the items to realize 
            //Generating the children from the item 
            //realizing the children
            //Measuring the children
            //Some further updates to other instance fields defined in ScrollCanvas
            //I left this out cause this sequence is more that 100 lines of code

            if (!_idleTimer.IsEnabled)
            {
                _idleTimer.Start();
            }
        }

    private void CleanUpItems(int minDesiredGenerated, int maxDesiredGenerated)
    {
        //This method uses same properties and resources as code in other lock that I left out for clarity
        var children = InternalChildren;
        var generator = ItemContainerGenerator;
        for (var i = children.Count - 1; i >= 0; i--)
        {
            var childGeneratorPos = new GeneratorPosition(i, 0);
            var itemIndex = generator.IndexFromGeneratorPosition(childGeneratorPos);
            if (itemIndex < minDesiredGenerated || itemIndex > maxDesiredGenerated)
            {
                generator.Remove(childGeneratorPos, 1);
                RemoveInternalChildRange(i, 1);
            }
        }
    }

    private void CleanUpItems(object sender, EventArgs eventArgs)
    {
        _idleTimer.Stop();
        lock (_idleTimer)
        {
            CleanUpItems(_protectedInterval.First, _protectedInterval.Last);
        }
    }
}

<Style TargetType="local:PageViewer">
    <Setter Property="Background"
            Value="Transparent"/>
    <Setter Property="VirtualizingStackPanel.IsVirtualizing"
            Value="True" />
    <Setter Property="ItemsPanel">
        <Setter.Value>
            <ItemsPanelTemplate>
                <local:ScrollCanvas Background="{Binding Background, RelativeSource={RelativeSource AncestorType=local:PageViewer}}" />
            </ItemsPanelTemplate>
        </Setter.Value>
    </Setter>
</Style>
user1182735
  • 764
  • 9
  • 21
  • 1
    The locking will never cause a deadlock, since methods are never interrupted in order to run other methods on the same thread. The thread itself may be pre-empted by the OS to give time for another thread to run, but that's transparent to that individual thread. Since you're correct that all methods run on the UI thread, a deadlock is impossible (and in fact, locking is merely useless overhead as you suspected). You've inherited some odd code :P – Cameron Jan 20 '15 at 17:16
  • 1
    I agree that with a 5 ms delay, a timer is no better than simply calling `Dispatcher.BeginInvoke()` (or `InvokeAsync()`). But according to a comment you placed in the code, you did not share all of the code that is protected by the `lock (_idleTimer)`, so we can't verify that code isn't called on some other thread. If you are _sure_ all of the code is executed in the UI thread, then deadlock is impossible and the synchronization provided by the `lock` statements is unnecessary. – Peter Duniho Jan 20 '15 at 18:01

0 Answers0