Ensure tile-loaded event completionCallback is called only once.

[Aiosa]

The PR also errors when context2D is set after cache creation.

[Aiosa]

Possible fix for #2277

[Aiosa]

Hmm not really friendly solution with tests :/

[Aiosa]

the issue is

            assert.ok( tile.loading, "The tile should be marked as loading.");
            assert.notOk( tile.loaded, "The tile should not be marked as loaded.");
            setTimeout(function() {
                assert.ok( tile.loading, "The tile should be marked as loading.");                <-- here the default completion callback has been already executed
                assert.notOk( tile.loaded, "The tile should not be marked as loaded.");
                callback1();
                assert.ok( tile.loading, "The tile should be marked as loading.");
                assert.notOk( tile.loaded, "The tile should not be marked as loaded.");
                setTimeout(function() {
                    assert.ok( tile.loading, "The tile should be marked as loading.");
                    assert.notOk( tile.loaded, "The tile should not be marked as loaded.");
                    callback2();
                    assert.notOk( tile.loading, "The tile should not be marked as loading.");
                    assert.ok( tile.loaded, "The tile should be marked as loaded.");
                    done();
                }, 0);
            }, 0);

so now it kind of works if the callback is not asynchronous, but does not work vice versa (as intended with tests). Also, the callback is called immediately instead of waiting for the last caller who got a completion callback. Not sure what is better or worse.

[Aiosa]

Implementation changed to use flags that guard completion (#2277 (comment)). Still there might be issue when early completion (completion called immediately), and another plugin attaches context2D. Possibly add also flag that prevents completion before the event finishes?

[iangilman]

Looks good to me. Just a quick comment on the error.

Still there might be issue when early completion (completion called immediately), and another plugin attaches context2D. Possibly add also flag that prevents completion before the event finishes?

Seems reasonable. I suppose we could just call getCompletionCallback before we send out the event, and then call that callback at the end. Come to think of it, that would probably make it so we don't need completed anymore!

[Aiosa]

In the end, I changed the error message as well as docs to be yet more explicit, it will not hurt. Using completion without async has no reason, now early completion can happen only when one hacks the semaphore with two completion calls.

[pearcetm]

This looks to me like it should work as intended. A couple of thoughts:

1. This would be a totally natural place to use the Promise API to resolve the loading process exactly one time using Promise.all(). I'm guessing the lack of IE support for Promise precludes this method though? (If IE support has been dropped, I have a Promise-based implementation coded up already).
2. An advantage of the Promise approach is that it is intrinsically robust to calling resolve more than once - subsequent invocations are simply ignored. This would guard against the cache having the same problem it does now, if someone inadvertently calls the callback multiple times. Even if we can't use Promise, I think it would be good to ensure the "completion" code runs just once, to protect against user-introduced bugs.
3. I do not think the comment warning against using getCompletionCallback for synchronous processing is needed. This new implementation can be called sync or async with equivalent results. This enables cleaner downstream code and APIs. I do not see the need to scare people off from using it just because its not necessary in the sync case.
4. Instead, I would be more explicit that this must be used for async processing, and is optional for synchronous processing.

[Aiosa]

@pearcetm I had just last week or so PR on promise support in events, which would solve this as well (the event could just behave as synchronized await Promise.all()). We decided not to include it at the end since it brings a lot of complexity without a strong motivation why it is really needed to be in the core library (I am still using it in my app by extending EventSource). So I guess it is still not a must-have reason and we rather keep the old API if possible, because compatibility.

Offtopic: to be honest, the first thing I would like with ES6 introduction would be to rewrite manual inheritance on all OSD classes 😄

I do not see the need to scare people off from using it just because its not necessary in the sync case

I do. The filtering plugin that started this PR does pretty wild coding to call the callback at the end of filter application - for no reason (except the bugs 😄 ). It lets them know: do not touch if you don't know you need it.

[pearcetm]

So I guess it is still not a must-have reason and we rather keep the old API if possible, because compatibility.

I saw that PR and discussion. In this case, though, it would be a purely internal use of Promise which would not change the public API at all; it really only relates to browser support, and does not expose any new complexity.

[pearcetm]

I do not see the need to scare people off from using it just because its not necessary in the sync case

I do. The filtering plugin that started this PR does pretty wild coding to call the callback at the end of filter application - for no reason (except the bugs 😄 ). It lets them know: do not touch if you don't know you need it.

I don't think the filtering plugin is "wild coding" - it is intended to support arbitrary combinations of sync and async filter operations. Since async is allowed, it gets a callback, and asks each filter to indicate when it's completed, and then calls the callback. To avoid getCompletionCallback for sync filters, it would have to change behavior depending on the exact filter stack. IMO it's a cleaner and more consistent API for developers for this to be allowed both sync and async.

[iangilman]

I agree that we don't need the warning against using it synchronously. It'll work just fine with this fix, and it's a valid use case.

We should definitely consider the possibility of introducing ES6. I'll start a new issue for that. Either way, I don't think we need a promise here... We already have an implementation that works, and adding the first promise is a big step, even if it's just internally.

As far as I'm concerned, I think this is ready to land once the "note" is removed from the docs.

It's amusing to me how simple this has become from what seemed like a complicated issue at first! It's great working through these things together :)

[pearcetm]

It's amusing to me how simple this has become from what seemed like a complicated issue at first! It's great working through these things together :)

Yup! The tough part was drilling down far enough to figure out the root cause. Other than improving documentation and error messages, the ultimate fix boils down to just 1 or 2 lines of code changes :)

We should definitely consider the possibility of introducing ES6. I'll start a new issue for that. Either way, I don't think we need a promise here... We already have an implementation that works, and adding the first promise is a big step, even if it's just internally.

I completely agree that since ES6 support hasn't been introduced yet, this isn't the hill to fight that battle on. As you say it isn't necessary since this implementation works. But the Promise-based approach does make for quite clean code, and if ES6 had been introduced already, it would have been almost "self-documenting" because it fits the needs of this function so closely. All that to say, I am in total support of the issue you opened about adding ES6!

[iangilman]

Lovely, thank you!