Modify

Opened 3 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#19532 closed defect (fixed)

IAE: Cannot modify the id counter backwards

Reported by: joris Owned by: team
Priority: major Milestone: 20.09
Component: Core Version:
Keywords: template_report Cc:

Description (last modified by Klumbumbus)

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)

Attachments (4)

19532-debug.patch (723 bytes) - added by GerdP 3 weeks ago.
19532.patch (1.1 KB) - added by GerdP 3 weeks ago.
workaound: create fake nodes with positive id so that generateUniqueId() isn't called
19532-not-parallel.patch (1.6 KB) - added by GerdP 3 weeks ago.
run "Initializing internal boundaries data" in beforeInitializationTasks so that we have no concurrent thread
19532-fix.patch (1.7 KB) - added by GerdP 3 weeks ago.
use atomic operation

Download all attachments as: .zip

Change History (31)

comment:1 Changed 3 months ago by Klumbumbus

Description: modified (diff)
Summary: Java illegal argument exceptionIAE: Cannot modify the id counter backwards

comment:2 Changed 3 months ago by skyper

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

comment:3 Changed 3 months ago by 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.

comment:4 Changed 3 months ago by 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.

comment:5 Changed 3 months ago by 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.

comment:6 Changed 3 months ago by 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.

comment:7 Changed 3 months ago by 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....

comment:8 Changed 3 months ago by 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?

comment:9 Changed 3 months ago by njsmurf1813

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

comment:10 Changed 3 months ago by 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?


comment:11 Changed 2 months ago by skyper

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

comment:12 Changed 2 months ago by Klumbumbus

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

comment:13 Changed 2 months ago by skyper

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

comment:14 Changed 2 months ago by simon04

Milestone: 20.08

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

comment:15 Changed 7 weeks ago by simon04

Milestone: 20.0820.09

comment:16 Changed 4 weeks ago by Klumbumbus

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

comment:17 Changed 3 weeks ago by skyper

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

comment:18 Changed 3 weeks ago by Don-vip

Priority: normalmajor

Changed 3 weeks ago by GerdP

Attachment: 19532-debug.patch added

comment:19 Changed 3 weeks ago by 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.

comment:20 Changed 3 weeks ago by 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	

Changed 3 weeks ago by GerdP

Attachment: 19532.patch added

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

Changed 3 weeks ago by GerdP

Attachment: 19532-not-parallel.patch added

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

comment:21 Changed 3 weeks ago by GerdP

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

comment:22 Changed 3 weeks ago by stoecker

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

comment:23 Changed 3 weeks ago by 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.

comment:24 Changed 3 weeks ago by Don-vip

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

comment:25 Changed 3 weeks ago by GerdP

Resolution: fixed
Status: newclosed

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.

Changed 3 weeks ago by GerdP

Attachment: 19532-fix.patch added

use atomic operation

comment:26 Changed 3 weeks ago by GerdP

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

comment:27 Changed 3 weeks ago by 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?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.