map.isSourceLoaded(...) may return true while a GeoJSON source is still loading

[vanilla-lake]

1.15.2:

Steps to Trigger Behavior

After the map.load event has fired, call GeoJSONSource.setData(...) twice, the first time passing a URL to a small GeoJSON file and the second time passing a URL to a large GeoJSON file.

Link to Demonstration

https://jsbin.com/colaxiboka/2/edit?html,output

Expected Behavior

• map.isSourceLoaded(...) should return false until the file specified in the last GeoJSONSource.setData(...) call has finished loading
• No points from the first GeoJSON file should be shown after the second call to GeoJSONSource.setData(...)

Actual Behavior

• map.isSourceLoaded(...) returns true between the times the files specified in the two GeoJSONSource.setData(...) calls finish loading
• Points from the first GeoJSON file are shown between the time of the second GeoJSONSource.setData(...) call and the time the second file finishes loading

[vanilla-lake]

I think the problem lies in the actor.send(...) callback in GeoJSONSource._updateWorkerData at the following line:

maplibre-gl-js/src/source/geojson_source.ts

Line 286 in d5d4e1d

this._loaded = true;

The actor.send(...) callback runs once for each GeoJSONSource.setData(...) call. Since the first file is small and should thus load quickly, the actor.send(...) callback for the first GeoJSONSource.setData(...) call is called quickly, setting the GeoJSONSource._loaded flag to true before the callback for the second call runs.

The problematic sequence is:

1. GeoJSONSource.setData(".../smaller.geojson") is called.
2. GeoJSONSource.setData(".../larger.geojson") is called.
3. The actor.send(...) callback for the GeoJSONSource.setData(".../smaller.geojson") call runs, setting GeoJSONSource._loaded to true. At this point, map.isSourceLoaded("geojson") returns true.
4. The actor.send(...) callback for the GeoJSONSource.setData(".../larger.geojson") call runs, setting GeoJSONSource._loaded to true one more time.

[HarelM]

This is an interesting analysis, thanks for sharing!
So the problem is some kind of race condition on the _loaded variable, right?
I see in the code that there's a check for abandoned, maybe two calls to setData should somehow trigger the abandoned for the first request. IDK...
In any case I would suggest to try and create a test that fails due to this race condition and see that it passes after you implement a fix.

[vanilla-lake]

No problem, and yes, it seems to be a race condition on GeoJSONSource._loaded. I'll try creating the test and see if I can work on a potential fix.

[vanilla-lake]

Based on the following unit test, it looks like the pending call is always expected to run to completion; only calls that have not yet started loading are abandoned when a new call is made:

maplibre-gl-js/test/unit/source/geojson_worker_source.test.js

Lines 225 to 245 in b2c300a

	t.test('abandons coalesced callbacks', (t) => {
	// Expect first call to run, second to be abandoned,
	// and third to run in response to coalesce
	const worker = createWorker();
	worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => {
	t.equal(err, null);
	t.notOk(result && result.abandoned);
	worker.coalesce({source: 'source1'});
	});
	worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => {
	t.equal(err, null);
	t.ok(result && result.abandoned);
	});
	worker.loadData({source: 'source1', data: JSON.stringify(geoJson)}, (err, result) => {
	t.equal(err, null);
	t.notOk(result && result.abandoned);
	t.end();
	});
	});

Based on that, should we show data from the previous pending call when it gets loaded while data from the latest one is still loading? In other words, should we show stale data from the previous GeoJSONSource.setData(...) call that will soon be replaced by that of the latest one?

[HarelM]

I believe we should present data from previous call to setData until a new data is ready, this is at least what I would expect.

[vanilla-lake]

Sounds good. Thanks!

[HarelM]

Fixed by #684

[vanilla-lake]

re: #669 (comment), do you think it could be worth adding a configuration option to GeoJSON sources that prevents stale data from being shown? It could be disabled by default.

Our use case is that we call GeoJSONSource.setData(...) with a URL generated from a user query. If the user issues a query while the results from a previous one are loading, we don't want the results from the first query to show up while the results from the second are loading.

[HarelM]

Personally, I would like to avoid complicating geojson with this kind of options... I'm not even sure where would one place such configuration option...
Can't you simply setData to empty array while waiting for the second request to load?

[vanilla-lake]

We tried doing the following:

// First query
GeoJSONSource.setData(url1);
// Second query (url1 is still loading)
GeoJSONSource.setData({type: "FeatureCollection", features: []});
GeoJSONSource.setData(url2);

Unfortunately, the GeoJSONSource.setData({type: "FeatureCollection", features: []}) call gets abandoned when GeoJSONSource.setData(url2) is executed because of the following code:

maplibre-gl-js/src/source/geojson_worker_source.ts

Lines 132 to 135 in ddc348f

	if (this._pendingCallback) {
	// Tell the foreground the previous call has been abandoned
	this._pendingCallback(null, {abandoned: true});
	}

[HarelM]

What about removing and re-adding the source in order to abandon the current call and remove the data?

[vanilla-lake]

I'd need to try it... My concern with that solution however is that, unless I'm wrong, all layers that use the source will need to be removed and re-added. While it's certainly possible to do that, it seems a bit cumbersome.