IAE: Cannot modify the id counter backwards

[joris]

What steps will reproduce the problem?

1. use josm to creat changes, upload and quit
2. restart soon, about one second after josm quitted

What is the expected result?

no exception

What happens instead?

exception

Please provide any additional information below. Attach a screenshot if possible.

19:15:57 2509 geo. java -jar josm-tested.jar
2020-07-14 19:16:00.088 INFO: Log level is at INFO (INFO, 800)
2020-07-14 19:16:03.539 INFO: Reassigning OSX shortcut 'file:downloadreferrers' from Meta to Ctrl because of conflict with ⌘+⌥+D
2020-07-14 19:16:03.539 INFO: Silent shortcut conflict: 'file:downloadreferrers' moved by 'apple-reserved-42' to '⌃+⌥+D'.
2020-07-14 19:16:03.715 INFO: Reassigning OSX shortcut 'core:historyinfo' from Meta to Ctrl because of conflict with ⌘+H
2020-07-14 19:16:03.716 INFO: Silent shortcut conflict: 'core:historyinfo' moved by 'system:hide' to '⌃+H'.
2020-07-14 19:16:06.008 INFO: GET https://api.openstreetmap.org/api/0.6/user/details -> HTTP/1.1 200 (1.9 s; 719 B)
2020-07-14 19:16:06.167 INFO: Reassigning OSX shortcut 'help:search-items' from Meta to Ctrl because of conflict with ⌘+␣
2020-07-14 19:16:06.167 INFO: Silent shortcut conflict: 'help:search-items' moved by 'apple-reserved-01' to '⌃+␣'.
2020-07-14 19:16:06.287 INFO: Obtained 68 Tag2Link rules from META-INF/resources/webjars/tag2link/2020.5.16/index.json
2020-07-14 19:16:08.733 SEVERE: Handled by bug report queue: org.openstreetmap.josm.tools.JosmRuntimeException: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards. Cause: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards. Cause: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
org.openstreetmap.josm.tools.JosmRuntimeException: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
	at org.openstreetmap.josm.spi.lifecycle.Lifecycle.initialize(Lifecycle.java:89)
	at org.openstreetmap.josm.gui.MainApplication.mainJOSM(MainApplication.java:915)
	at org.openstreetmap.josm.gui.MainApplication$3.processArguments(MainApplication.java:276)
	at org.openstreetmap.josm.gui.MainApplication.main(MainApplication.java:713)
Caused by: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
	at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
	at org.openstreetmap.josm.spi.lifecycle.Lifecycle.initialize(Lifecycle.java:78)
	... 3 more
Caused by: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
	at org.openstreetmap.josm.data.osm.UniqueIdGenerator.advanceUniqueId(UniqueIdGenerator.java:38)
	at org.openstreetmap.josm.io.AbstractReader.buildPrimitive(AbstractReader.java:627)
	at org.openstreetmap.josm.io.AbstractReader.addNode(AbstractReader.java:638)
	at org.openstreetmap.josm.io.AbstractReader.parseNode(AbstractReader.java:680)
	at org.openstreetmap.josm.io.OsmReader.parseNode(OsmReader.java:230)
	at org.openstreetmap.josm.io.OsmReader.parseOsm(OsmReader.java:169)
	at org.openstreetmap.josm.io.OsmReader.parseRoot(OsmReader.java:135)
	at org.openstreetmap.josm.io.OsmReader.parse(OsmReader.java:121)
	at org.openstreetmap.josm.io.OsmReader.lambda$doParseDataSet$0(OsmReader.java:491)
	at org.openstreetmap.josm.io.AbstractReader.doParseDataSet(AbstractReader.java:298)
	at org.openstreetmap.josm.io.OsmReader.doParseDataSet(OsmReader.java:488)
	at org.openstreetmap.josm.io.OsmReader.parseDataSet(OsmReader.java:538)
	at org.openstreetmap.josm.io.OsmReader.parseDataSet(OsmReader.java:521)
	at org.openstreetmap.josm.tools.Territories.initializeInternalData(Territories.java:137)
	at org.openstreetmap.josm.tools.Territories.initialize(Territories.java:123)
	at org.openstreetmap.josm.gui.MainInitialization.lambda$parallelInitializationTasks$6(MainInitialization.java:114)
	at org.openstreetmap.josm.spi.lifecycle.InitializationTask.call(InitializationTask.java:33)
	at org.openstreetmap.josm.spi.lifecycle.InitializationTask.call(InitializationTask.java:11)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

2020-07-14 19:16:11.585 INFO: GET https://josm.openstreetmap.de/tested -> HTTP/1.1 200 (1.9 s)

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-06-30 19:13:42 +0200 (Tue, 30 Jun 2020)
Revision:16731
Build-Date:2020-07-01 01:30:51
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (16731 en_AU) Mac OS X 10.14.6
OS Build number: Mac OS X 10.14.6 (18G103)
Memory Usage: 336 MB / 4096 MB (251 MB allocated, but free)
Java version: 11.0.4+10-LTS, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Look and Feel: com.apple.laf.AquaLookAndFeel
Screen: Display 69733632 1440x900 (scaling 2.0x2.0), Display 724044302 3840x2160 (scaling 1.0x1.0)
Maximum Screen Size: 3840x2160
Best cursor sizes: 16x16 -> 16x16, 32x32 -> 32x32

Last errors/warnings:
- E: Handled by bug report queue: org.openstreetmap.josm.tools.JosmRuntimeException: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards. Cause: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards. Cause: java.lang.IllegalArgumentException: Cannot modify the id counter backwards


=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: main (1)
org.openstreetmap.josm.tools.JosmRuntimeException: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
	at org.openstreetmap.josm.spi.lifecycle.Lifecycle.initialize(Lifecycle.java:89)
	at org.openstreetmap.josm.gui.MainApplication.mainJOSM(MainApplication.java:915)
	at org.openstreetmap.josm.gui.MainApplication$3.processArguments(MainApplication.java:276)
	at org.openstreetmap.josm.gui.MainApplication.main(MainApplication.java:713)
Caused by: java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
	at java.base/java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.base/java.util.concurrent.FutureTask.get(FutureTask.java:191)
	at org.openstreetmap.josm.spi.lifecycle.Lifecycle.initialize(Lifecycle.java:78)
	... 3 more
Caused by: java.lang.IllegalArgumentException: Cannot modify the id counter backwards
	at org.openstreetmap.josm.data.osm.UniqueIdGenerator.advanceUniqueId(UniqueIdGenerator.java:38)
	at org.openstreetmap.josm.io.AbstractReader.buildPrimitive(AbstractReader.java:627)
	at org.openstreetmap.josm.io.AbstractReader.addNode(AbstractReader.java:638)
	at org.openstreetmap.josm.io.AbstractReader.parseNode(AbstractReader.java:680)
	at org.openstreetmap.josm.io.OsmReader.parseNode(OsmReader.java:230)
	at org.openstreetmap.josm.io.OsmReader.parseOsm(OsmReader.java:169)
	at org.openstreetmap.josm.io.OsmReader.parseRoot(OsmReader.java:135)
	at org.openstreetmap.josm.io.OsmReader.parse(OsmReader.java:121)
	at org.openstreetmap.josm.io.OsmReader.lambda$doParseDataSet$0(OsmReader.java:491)
	at org.openstreetmap.josm.io.AbstractReader.doParseDataSet(AbstractReader.java:298)
	at org.openstreetmap.josm.io.OsmReader.doParseDataSet(OsmReader.java:488)
	at org.openstreetmap.josm.io.OsmReader.parseDataSet(OsmReader.java:538)
	at org.openstreetmap.josm.io.OsmReader.parseDataSet(OsmReader.java:521)
	at org.openstreetmap.josm.tools.Territories.initializeInternalData(Territories.java:137)
	at org.openstreetmap.josm.tools.Territories.initialize(Territories.java:123)
	at org.openstreetmap.josm.gui.MainInitialization.lambda$parallelInitializationTasks$6(MainInitialization.java:114)
	at org.openstreetmap.josm.spi.lifecycle.InitializationTask.call(InitializationTask.java:33)
	at org.openstreetmap.josm.spi.lifecycle.InitializationTask.call(InitializationTask.java:11)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.base/java.lang.Thread.run(Thread.java:834)

[skyper]

Ticket #19548 has been marked as a duplicate of this ticket.

[njsmurf1813]

I was doing some debugging on a local copy of JOSM. My understanding is that the UniqueIdGenerator's counter is per osm type (node, way, relation) but not per dataset.

So, territories could be updating the node counter at the same time joris's edits are being loaded.
So, what I imagine is happening with this bug is the territories file is at say node id -3, joris's dataset is parsed and ends up setting the counter lower than -3 before my series of boundary ids has looped from -3 to -4.
Then, the condition on UniqueIdGenerator.java:39 is true and we get the error.

What do we think about making a lookup table so that we have the data structure
{

[datsetA] -> {

node -> uniqueIdGenerator,
way -> uniqueIdGenerator,
relation -> uniqueIdGenerator

},
[datsetB] -> {

node -> uniqueIdGenerator,
way -> uniqueIdGenerator,
relation -> uniqueIdGenerator

}

}

If this sounds interesting, happy to implement and add tests.

[skyper]

See #6582 and #13036 for tickets about ID per layer.

Is it really the problem here or is it a problem about timing or deleting cache on exit.

[njsmurf1813]

You are totally making me second guess as I am trying to create a test failure by having an executor kick off a whole bunch of runnables that parse different datasets to make this occur. It is, not occurring...

Is there a reason we can't remove this boundaries file from cache each exit? Like whether cached or from the jar archive the same thing happens "I think" when josm loads? It reads in osm data from a file on disk then parses it.

[GerdP]

Your approach regarding a distinct UniqueGenerator per type and dataset sounds good to me. I see no problems regarding the tickets mentioned in comment:4.

[njsmurf1813]

One potential hurdle I see is that there is a constructor in the NodeData/WayData/RelationData classes that use this uniqueIdGenerator and are not coupled to a dataset.

https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/data/osm/NodeData.java#L25

A quick and dirty thing for these would just be to add a dummy entry to the map called "test_data" and for any of these pass along that name. Not sure if we feel that is a super wrong thing to do....

[GerdP]

Isn't it possible to block the load of other datasets while the initialisation of territories isn't finished?
Maybe my change in r16552 has made it more likely to happen that initializeInternalData() isn't ready?

[njsmurf1813]

Would putting it in the runInitializationTasks + synchronizing the external data init function achieve this?

[njsmurf1813]

Also just regarding the per dataset idea, that is proving harder/more brittle than I'd hoped/imagined.

Firstly, datasets don't always have names so creating a hashmap of datasetName->counter is a recipe for null pointer errors as I found.

To get beyond that I said ok, I will just add a new uniqueId field to the dataset class that I can always set, expect to exist on each and every dataset, and use that to manage the datasetName->counter relationship.

This illuminated another problem - primitives' dataset field can also null.
This becomes problematic when we parse the boundaries. There is a code path that can looks up the dataset on the node object which has not yet been set to reference the dataset we will apply parsed data too.

I fear the interwoven here makes doing this a recipe for introducing new bugs. is it possible that we can update the condition that breaks so that it doesn't abide by the same rule if the 2 ids we are checking are negative?

[skyper]

Ticket #19678 has been marked as a duplicate of this ticket.

[Klumbumbus]

Ticket #19682 has been marked as a duplicate of this ticket.

[skyper]

Ticket #19687 has been marked as a duplicate of this ticket.

[simon04]

Do we have an easy and reliable way to reproduce this exception?

[Klumbumbus]

Ticket #19838 has been marked as a duplicate of this ticket.

[skyper]

Ticket #19861 has been marked as a duplicate of this ticket.

[GerdP]

With this small patch and a breakpoint on the line

long dd = 4;

I am able to reproduce the problem in eclipse.
The id -101747 is that of the last node in boundaries.osm.

[GerdP]

Seems that we have a concurrent thread that may create nodes:

MapCSSStyleSource.constructSpecial(String) line: 326	
MapCSSStyleSource.loadMeta() line: 244	
MapCSSStyleSource.loadStyleSource(boolean) line: 174	
MapCSSStyleSource(StyleSource).loadStyleSource() line: 112	
MapPaintStyles.loadStyleForFirstTime(StyleSource) line: 314	
MapPaintStyles.readFromPreferences() line: 304	
MapPaintPreference.initialize() line: 187	

[GerdP]

workaound: create fake nodes with positive id so that generateUniqueId() isn't called

[GerdP]

run "Initializing internal boundaries data" in beforeInitializationTasks so that we have no concurrent thread

[GerdP]

It seems safer to do it like this even if startup time might increase.

[stoecker]

The second variant sounds better to me. Who knows what side effects the "workaround" will have in future when nobody expects it anymore.

[GerdP]

A proper solution might be to move the if then else in buildPrimitive() to UniqueIdGenerator where it could use one of the Atomic operations. I just have no clue how.

[Don-vip]

+1 with the non-concurrent initialization, that's the safer approach. Can you please commit your patch?

[GerdP]

In 17080/josm:

fix #19532:IAE: Cannot modify the id counter backwards

• run "Initializing internal boundaries data" in beforeInitializationTasks so that we have no concurrent thread

This just avoids the situation were the problem occured, the problem itself still needs to be fixed.

[GerdP]

use atomic operation

[GerdP]

I've not tried this patch with any unit test but it seems to work fine with normal operations.

[GerdP]

IIGTR this is a regression that was introduced with the accidential commit in r14535 (#16854).
The implemented solution in r17080 only works for files which have negative ids that are lower than the current value stored in the UniqueIdGenerator. The actual values in these generators depend on various things that happen during startup, esp. the loading of the file boundaries.osm in class Territories and the mappaint styles.
Even if you happen to have a file that is loaded with the given ids this will only work once in each JOSM session.

Maybe we should better revert those changes instead of finding fixes for the problems?