das-developers / das2java

The original das2 library. Provides interactive publication-ready 2-D plotting
https://das2.org
GNU Lesser General Public License v3.0
4 stars 0 forks source link

TCA loading doesn't properly block create png #32

Closed jbfaden closed 2 years ago

jbfaden commented 2 years ago

For years I've dealt with a problem where adding TCAs (ephemeris) to an image can result in slightly unpredictable images when scripting and generating PNGWalks. There's a mechanism which should block the "waitUntilIdle" calls that is failing as TCAs are loaded.

This was originally created as an Autoplot bug, but the bug is really in Das2. See https://sourceforge.net/p/autoplot/bugs/2442/.

jbfaden commented 2 years ago

The code "updateTCAImmediately" exists within a block where a change is registered, and no image should be allowed. However repeatedly running the test I can see where sometimes getCanvas().isPendingChanges() will return false, though this should not happen.

Here I've modified the code to indicate isPendingChanges:

    /**
     * update the TCA (ephemeris) axis on the current thread.
     * The lock will have pendingChange and changePerformed with it.
     */
    private void updateTCAImmediately( ) {
        logger.fine("enter updateTCAImmediately...");
        System.err.println("pendingChanges: "+ getCanvas().isPendingChanges() );
        setBackground( Color.BLUE.brighter() );
        setForeground( Color.RED );
        synchronized (this) {
            logger.fine("...got lock.");
            final DasProgressWheel tcaProgress= new DasProgressWheel();
            tcaProgress.started();
            Runnable run= () -> {
                tcaProgress.getPanel(DasAxis.this);
            };
            SwingUtilities.invokeLater(run);

            try {
                updateTCADataSet();
            } finally {
                tcaProgress.finished();
                setBackground( Color.WHITE );
                setForeground( Color.BLACK );
                repaint();
            }
        }
        System.err.println("pendingChanges: "+ getCanvas().isPendingChanges() );
    }

and here is the code which calls:

    /**
     * update the TCA dataset.  This will load the TCAs on a RequestProcessor thread sometime soon.
     */
    private void updateTCASoon() {
        final DasCanvas lcanvas= getCanvas();
        logger.log(Level.FINE, "updateTCASoon {0}", lcanvas);
        if ( lcanvas!=null ) {
            lcanvas.registerPendingChange( this, tcaLock );
            RequestProcessor.invokeLater(new Runnable() {
                @Override
                public void run() {
                   lcanvas.performingChange( DasAxis.this, tcaLock );
                   try {
                       updateTCAImmediately( );
                   } finally {
                       lcanvas.changePerformed( DasAxis.this, tcaLock );
                   }
                }
            }, DasAxis.this );
        }
    }

and I can see that usually this has "pendingChanges: true", but sometimes it's false and that's when the image is mispainted.

jbfaden commented 2 years ago

See also https://github.com/autoplot/dev/blob/master/bugs/2022/20220324/demoBugSlowTCA.vap , which should be loaded, and then run a pngwalk with:

product=product
timeFormat=$Y$m
timeRange=2016-11-01/2017-01-01
autorange=true
jbfaden commented 2 years ago

There's a curious message in ChangesSupport.java's registerPendingChange: "Note, it is okay to call this multiple times for the same client and lock object." It does count up the number of calls for different lock objects, but not the same one. This logic is confusing and looks wrong.

jbfaden commented 2 years ago

I believe the gist of the problem is that the change is registered, but then a tickleTimer is used to perform the work. The tickleTimer will fire once, even though the pending change might be registered multiple times. There is code that tries to address this that needs to be studied (DasAxis.maybeStartTcaTimer).

jbfaden commented 2 years ago

I couldn't find any real problems caused by that, but it did occur to me that the same lock object is used in several places, which is not necessary. As an experiment, I use three separate lock objects and this seems to fix the problem. This still needs more rigorous testing, and I'll be doing that now.

jbfaden commented 2 years ago

That doesn't work either, since different threads would need to trigger the tcaTimer. I don't think changeSupport should ever be used with tickleTimers, since the tickleTimer is all about coalescing events.

jbfaden commented 2 years ago

This was resolved by making it so that dasCanvas waitUntilIdle also checks the isDirty flags on each component. Surprisingly, there are some dirty flags which were set but never cleared, so this needed work as well. The isDirty check on the DasAxis will return true when it is busy loading TCA data. This appears to fix the problem, and will be available in the next production release of Autoplot, and tested in the field with development releases.

jbfaden commented 2 years ago

This is fixed