[Patch] Background uploading

[Zverikk]

We already can download GPS traces and some objects in background while editing. Why not extend this to uploading data? Of course, after clicking "background" the layer being uploaded should be made read-only, but at least it should be possible to pan/zoom and change other layers.

This function should be disabled for "save on exit", of course.

[Don-vip]

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

[Don-vip]

Background tasks is clearly an improvement JOSM should provide in the future, there are already a lot of tickets related to this. The solution should be generic for all asynchronous tasks (download, upload, validation etc.)

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_0

[udit]

I created another class which allows Uploads to be moved to background. The patch can be found above. The class also marks the primitives to be updated (and their referrers) as disabled. While the upload is happening the user can create/edit/delete other primitives from JOSM.
However I can see some holes in this approach. Please feel free to add more to the list.

1. The primitives are just disabled & not locked. I have not gone in depth into the filtering process but I have a feeling that by applying the right filter these primitives can be enabled pre-maturely
2. I could not tested this class against conflicts. Although the entire upload flow is delegated to the UploadPrimitiveTask class there shouldn't be any difference in behavior, but without tests I am not sure.

[udit]

Another approach that I tried was to lock the dataset for the upload process. That allows the user to interact with JOSM, but not the layer being uploaded. However this approach would require couple of changes

1. In cases of conflict the current edit layer is searched for the conflicting primitives. This would need to change to look for the correct layer instead of just assuming that the current layer has the conflicts.
2. The post upload cleanup task in UploadPrimitivesTask class happens on EDT. This makes the app go into deadlock since the lock acquired on dataset is taken in the worker threads.

[Zverikk]

An option to make a dataset read-only would be a great feature, regardless of this ticket. Maybe this should even be available from a layer popup menu, to prevent accidental changes to a reference layer, for example.

[Don-vip]

The dataset-locking approach sounds safer & easier to maintain. I agree with the idea of manually locking a layer, could be useful.

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_1

[udit]

The new patch does the following on an upload action:
1- Create a transient OsmDataLayer with empty dataset. Sets this layer as the current EditLayer
2- Lock the original OsmDataLayer and proceed with an asynchronous upload.
3- The user cannot edit anything on the original layer, however he's free to interact with JOSM and create new primitives on the transient layer
4- When the upload finishes (or cancels or conflicts) the transient layer is merged into the upload layer. Then the subsequent action is taken.

Let me know if this requires further changes. One possible downside of this patch is that users can't edit during upload but they can create new stuff.

[bastiK]

This business with creating a transient layer and merging it automatically after finished upload is a bit too complicated for my taste.

It would be simpler to just disable the current data layer for the time of the upload (perhaps with some kind of on-screen message). Leave it to the advanced user to open a new layer and continue editing if they are comfortable doing so.

Currently it prints

WARNING: Cannot paint layer {0}: It is locked.

to the command line. This is probably not how it is supposed to work. The layer could be painted in disabled style like a non-active data layer.

[Zverikk]

I agree with BastiK. A transient layer is a path to frustration and conflicts. Could you please try implementing a read-only locking for a data layer?

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_2.

[udit]

async_v2.patch implements locking on OsmDataLayer.

It also modifies the MainLayerManager 's getters for active and edit layers. When a layer is marked as read only and if it was the current active/edit layer MainLayerManager would proceed to return a null value. Thus no action can be taken while the layer is marked as read only. There are also checks against making a read only layer as the active/edit layer.

If the user wishes to continue mapping while the upload is happening in background she can do so by creating a new layer manually as suggested by @bastiK

[bastiK]

Looks okay, unless you try to interact with the upload-in-progress layer. Then you can trigger all kind of errors (e.g. "select all", edit a relation in the relations panel, ...). My feeling is that this is a pretty extensive task you've taken on.

I've tried to download a new area into the current layer while the current upload is in progress. It seems to queue the task and perform it properly after the upload is finished. Likewise, it may just work to queue several uploads (from other layers). So the error dialog could be changed into a warning/confirmation.

[udit]

Replying to bastiK:

Looks okay, unless you try to interact with the upload-in-progress layer. Then you can trigger all kind of errors (e.g. "select all", edit a relation in the relations panel, ...). My feeling is that this is a pretty extensive task you've taken on.

Hey @bastiK can you tell me how you were able to trigger errors? I tried editing a test relation using the relation panel during upload, but I couldn't change anything.
Ideally the user should not be able to interact with the upload-in-progress layer at all. Any primitives on the upload-in-progress layer should not be editable from the app. But technically the dataset is not locked as such and things can be added if an object has reference to the dataset. I can inspect that further.

I've tried to download a new area into the current layer while the current upload is in progress. It seems to queue the task and perform it properly after the upload is finished. Likewise, it may just work to queue several uploads (from other layers). So the error dialog could be changed into a warning/confirmation.

As for queuing multiple uploads I think that can be tackled later. Asynchronous upload should be fixed first. Once it is done and validated, extending it to allow multiple uploads should be easy enough.

[bastiK]

Replying to udit:

Replying to bastiK:

Looks okay, unless you try to interact with the upload-in-progress layer. Then you can trigger all kind of errors (e.g. "select all", edit a relation in the relations panel, ...). My feeling is that this is a pretty extensive task you've taken on.

Hey @bastiK can you tell me how you were able to trigger errors? I tried editing a test relation using the relation panel during upload, but I couldn't change anything.
Ideally the user should not be able to interact with the upload-in-progress layer at all. Any primitives on the upload-in-progress layer should not be editable from the app. But technically the dataset is not locked as such and things can be added if an object has reference to the dataset. I can inspect that further.

In the relation panel, click on one of the relations in the upload-in-progress layer, then click "call relation editor" button (2nd button). The error window should come up:

java.lang.IllegalArgumentException: Parameter 'layer' must not be null
	at org.openstreetmap.josm.tools.CheckParameterUtil.ensureParameterNotNull(CheckParameterUtil.java:135)
	at org.openstreetmap.josm.gui.dialogs.relation.RelationEditor.<init>(RelationEditor.java:66)
	at org.openstreetmap.josm.gui.dialogs.relation.GenericRelationEditor.<init>(GenericRelationEditor.java:182)
	at org.openstreetmap.josm.gui.dialogs.relation.RelationEditor.getEditor(RelationEditor.java:116)
	at org.openstreetmap.josm.actions.relation.EditRelationAction.launchEditor(EditRelationAction.java:66)
	at org.openstreetmap.josm.actions.relation.EditRelationAction.actionPerformed(EditRelationAction.java:83)
	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2022)

Alternative is clicking "Tools > Add node..." and on confirmation I get

Thread: AWT-EventQueue-1 (21) of main
java.lang.IllegalArgumentException: Parameter 'data' must not be null
	at org.openstreetmap.josm.tools.CheckParameterUtil.ensureParameterNotNull(CheckParameterUtil.java:135)
	at org.openstreetmap.josm.command.Command.<init>(Command.java:173)
	at org.openstreetmap.josm.command.AddCommand.<init>(AddCommand.java:64)
	at org.openstreetmap.josm.actions.AddNodeAction.actionPerformed(AddNodeAction.java:70)
	at javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:2022)

I've tried to download a new area into the current layer while the current upload is in progress. It seems to queue the task and perform it properly after the upload is finished. Likewise, it may just work to queue several uploads (from other layers). So the error dialog could be changed into a warning/confirmation.

As for queuing multiple uploads I think that can be tackled later. Asynchronous upload should be fixed first. Once it is done and validated, extending it to allow multiple uploads should be easy enough.

That's true.

Edit: Maybe also lower priority, but when exiting JOSM, a warning about on-going upload would be nice.

[udit]

The new patch removes the upload-in-progress layer from the active/edit layer reference of MainApplication. If there is another data layer present, then that layer is promoted as the new active+edit layer. Otherwise if only a non-data layer is present, then that is promoted as the active layer and the edit layer is set to empty. In the case where no other layer is present except for the upload-in-progress layer then both active and edit layer are set to empty.

Setting the edit layer to empty disables all the edit action in JOSM. Hence the layer is safe guarded against changes during upload. Once the upload is finished the upload-in-progress layer is promoted to active+edit layer iff both of them are empty. If active layer is set but not the edit layer (think of the case where we have one imagery layer and the other is upload-in-progress layer) then edit layer is set to the upload-in-progress layer. If both active and edit layer are not empty then no change happens.

This change also introduces a configuration in advanced preferences. I thought it would be wise for to allow users to switch off the async upload in case they are facing problems. Adding a "asynchronous.upload" key the value "true" in advanced preferences pane would enable async upload.

Another possible case of failure was the undoRedo operations. The undoRedo operations pertaining to the upload-in-progress layer are now cleared so that there is no error.

[bastiK]

Okay, the exceptions are gone now!

Replying to udit:

The new patch removes the upload-in-progress layer from the active/edit layer reference of MainApplication. If there is another data layer present, then that layer is promoted as the new active+edit layer. Otherwise if only a non-data layer is present, then that is promoted as the active layer and the edit layer is set to empty. In the case where no other layer is present except for the upload-in-progress layer then both active and edit layer are set to empty.

Setting the edit layer to empty disables all the edit action in JOSM. Hence the layer is safe guarded against changes during upload. Once the upload is finished the upload-in-progress layer is promoted to active+edit layer iff both of them are empty. If active layer is set but not the edit layer (think of the case where we have one imagery layer and the other is upload-in-progress layer) then edit layer is set to the upload-in-progress layer. If both active and edit layer are not empty then no change happens.

There are still problems with this system: Take the common case of one osm layer and one Bing layer and the user uploads a very small change, without clicking the "run in background" button. Then the active layer will switch to Bing and not return back. This is irritating.

The way you set the editLayer to null is right, but why can't we keep the active layer as is? Of course, some kind of indication that the layer is in a locked state would be nice. For example replacing the green check mark in the layer list panel by another symbol.

This change also introduces a configuration in advanced preferences. I thought it would be wise for to allow users to switch off the async upload in case they are facing problems. Adding a "asynchronous.upload" key the value "true" in advanced preferences pane would enable async upload.

Okay, but then it should be turned on by default. We do not include major features that are only accessible by setting an advanced preference value.

[udit]

Replying to bastiK:

Okay, the exceptions are gone now!

Replying to udit:

The new patch removes the upload-in-progress layer from the active/edit layer reference of MainApplication. If there is another data layer present, then that layer is promoted as the new active+edit layer. Otherwise if only a non-data layer is present, then that is promoted as the active layer and the edit layer is set to empty. In the case where no other layer is present except for the upload-in-progress layer then both active and edit layer are set to empty.

Setting the edit layer to empty disables all the edit action in JOSM. Hence the layer is safe guarded against changes during upload. Once the upload is finished the upload-in-progress layer is promoted to active+edit layer iff both of them are empty. If active layer is set but not the edit layer (think of the case where we have one imagery layer and the other is upload-in-progress layer) then edit layer is set to the upload-in-progress layer. If both active and edit layer are not empty then no change happens.

There are still problems with this system: Take the common case of one osm layer and one Bing layer and the user uploads a very small change, without clicking the "run in background" button. Then the active layer will switch to Bing and not return back. This is irritating.

I have changed this so that the active layer is not changed during/after upload.

The way you set the editLayer to null is right, but why can't we keep the active layer as is? Of course, some kind of indication that the layer is in a locked state would be nice. For example replacing the green check mark in the layer list panel by another symbol.

I tried coming up with a way to change the active layer icon to something else (I settled on the existing "clock" icon to depict waiting). However I couldn't figure out a clean way to change the active layer icon to that. The ActiveLayerCheckBox is used to display the active/inactive check box in LayerListDialog. To change the icon on upload would require instantiating a new instance of ActiveLayerCheckBox with appropriate checked icon. However the problem now is that if we select any other layer as active that icon would be used instead of the original green tick.

As a work around I propose to do the image overlay that happens when you mark a layer as "Discourage Upload". Its easy enough to override the icon in that case because it is specific to the layer. Or I can change the icon of the data layer till its upload is done (which is what I did in the latest patch. The overlay wasn't very explicit)

This change also introduces a configuration in advanced preferences. I thought it would be wise for to allow users to switch off the async upload in case they are facing problems. Adding a "asynchronous.upload" key the value "true" in advanced preferences pane would enable async upload.

Okay, but then it should be turned on by default. We do not include major features that are only accessible by setting an advanced preference value.

I have reversed the configuration logic. Async upload is enabled by default but the user can go back to blocking upload if she wishes to. The configuration in that case would be "asynchronous.upload" = "disabled"

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_4.

[bastiK]

Replying to udit:

The way you set the editLayer to null is right, but why can't we keep the active layer as is? Of course, some kind of indication that the layer is in a locked state would be nice. For example replacing the green check mark in the layer list panel by another symbol.

I tried coming up with a way to change the active layer icon to something else (I settled on the existing "clock" icon to depict waiting). However I couldn't figure out a clean way to change the active layer icon to that. The ActiveLayerCheckBox is used to display the active/inactive check box in LayerListDialog. To change the icon on upload would require instantiating a new instance of ActiveLayerCheckBox with appropriate checked icon. However the problem now is that if we select any other layer as active that icon would be used instead of the original green tick.

As a work around I propose to do the image overlay that happens when you mark a layer as "Discourage Upload". Its easy enough to override the icon in that case because it is specific to the layer. Or I can change the icon of the data layer till its upload is done (which is what I did in the latest patch. The overlay wasn't very explicit)

This is okay, I guess. An alternative would be to repurpose the on-screen message that is displayed when filters are active (class OSDLabel). It would be more visible than the small icon in the layer list.

One more minor thing: The use of ReentrantReadWriteLock seems unnecessary. As far as I can tell, it can be replaced by a simple boolean field with synchronized modifier for the access methods (or AtomicBoolean).

[udit]

This is okay, I guess. An alternative would be to repurpose the on-screen message that is displayed when filters are active (class OSDLabel). It would be more visible than the small icon in the layer list.

The on-screen message on filters would definitely be useful for the end user. Unfortunately I couldn't get it to work for our use case. I can try to spend some more time on this tomorrow. However incase I am not able to come up with a fix would it make sense to move it to a separate issue? Unless you think its a blocker for this patch.

One more minor thing: The use of ReentrantReadWriteLock seems unnecessary. As far as I can tell, it can be replaced by a simple boolean field with synchronized modifier for the access methods (or AtomicBoolean).

I've changed this in the newest patch.

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_5.

[bastiK]

Replying to udit:

This is okay, I guess. An alternative would be to repurpose the on-screen message that is displayed when filters are active (class OSDLabel). It would be more visible than the small icon in the layer list.

The on-screen message on filters would definitely be useful for the end user. Unfortunately I couldn't get it to work for our use case. I can try to spend some more time on this tomorrow. However incase I am not able to come up with a fix would it make sense to move it to a separate issue? Unless you think its a blocker for this patch.

Looks good, we can commit the current state. One thing I forgot to ask: please add javadoc for every public method (except unit tests and methods with @Override annotation).

[Don-vip]

Please also add @since xxx in javadoc of new classes or new public methods of existing classes. We'll replace xxx by the SVN revision when applying the patch. To check javadoc is OK you can run ant checkstyle.

[udit]

Checkstyle is warning that I declare the AsynchronousUploadPrimitivesTask as final. But why?

[Don-vip]

It's this check I suppose: ​http://checkstyle.sourceforge.net/config_design.html#FinalClass

Checks that a class which has only private constructors is declared as final. Doesn't check for classes nested in interfaces or annotations, as they are always final there.

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_6.

[udit]

Replying to Don-vip:

It's this check I suppose: ​http://checkstyle.sourceforge.net/config_design.html#FinalClass

Checks that a class which has only private constructors is declared as final. Doesn't check for classes nested in interfaces or annotations, as they are always final there.

Ah that makes sense. I have updated the Javadoc accordingly. No checkstyle errors now.

[bastiK]

Javadoc for public methods is still missing in async_v6.patch.

[udit]

Replying to bastiK:

Javadoc for public methods is still missing in async_v6.patch.

Sorry about that. Fixed in the latest patch.

[udit]

Patch for Asynchronous uploads in JOSM. This is a version_7.

[bastiK]

In 13133/josm:

applied #8509 - Background uploading (patch by udit, minor changes)

[cmuelle8]

Sorry for being late to this, I see it is merged.

I've read through the ticket and thought about the following alternative approach:

• duplicate the dataset of the active layer
• upload the duplicate async
• keep editing the active layer (no locks at all), which might produce conflicts
• Make the conflicts available to the user to fix (if any) by
  • either doing UpdateData action on the active layer
  • or simply merging the datasets offline (merging is able to fill the conflict dialog just as UpdateData would)

A downside of doing the verification pass online is increased traffic, but an additional upside would be inclusion of eventual changes done by peer mappers in the meantime.

Overall this employs the conflict detection engine in JOSM to keep the dataset editable, i.e. the dataset is not simply unlocked after a successful upload but needs to be merged with a new dataset (either the local duplicate or the polled data in result of UpdateData Action).

It's just a rough draft, it might not be feasible to do, but it seems this has not been thought about in above discussion (or maybe I've overlooked, sorry in this case).

If the server does not accept the changeset, this might produce additional complexity though: A conflict object produced as a result of a failed upload might be difficult to merge to the active dataset if it involves primitives that have been changed ("conflict hitting conflict" case). In this case it may be better to not try to merge the dataset with the failed upload, but to issue UpdateData on the active layer instead and simply warn the user that the upload attempt failed. I.e. the merge of duplicate and active layer dataset should only be necessary, if the upload succeeded. So it may be harder to code than the locking approach, I suppose.

[udit]

Replying to cmuelle8:

I've read through the ticket and thought about the following alternative approach:

• duplicate the dataset of the active layer
• upload the duplicate async
• keep editing the active layer (no locks at all), which might produce conflicts
• Make the conflicts available to the user to fix (if any) by
  • either doing UpdateData action on the active layer
  • or simply merging the datasets offline (merging is able to fill the conflict dialog just as UpdateData would)

The transient layer approach of async_v1.patch sort of implements your idea. Instead of creating a duplicate dataset it created a duplicate layer which the user can keep editing while the upload happens on the original layer. What are your thoughts on that?

[cmuelle8]

Replying to udit:

The transient layer approach of async_v1.patch sort of implements your idea. Instead of creating a duplicate dataset it created a duplicate layer which the user can keep editing while the upload happens on the original layer. What are your thoughts on that?

Yes, seems like v1 approach described in comment:8 was quickly put off in comment:9 and comment:10. But you were trying it the other way around: i.e. make a new empty layer editable, instead of just leaving the active edit layer in editing state.

To really do this without locking (i.e. seamless to the user), one needs

• to make a snapshot of the data to upload
  • snapshot = duplicate of the dataset or maybe osc file
• use that snapshot for upload in async thread that creates events upon completion like this
  • UploadSuccessfulEvent (i.e. merge duplicate to active edit layer or use UpdateData Action)
  • UploadFailedEvent (this essentially means merging the conflicts (that resulted from upload) to the active edit layer, I'd think)

The hard thing to get right is handling UploadFailedEvent.

• if the primitive(s) involved in the conflict has(have) not changed in the active edit layer in the meantime, its easy to merge the conflict, i.e. the conflict resulting from the upload attempt, to the active edit layer
• but how to handle conflicts that refer to primitives that have changed (for instance, have been deleted in the active edit layer in the meantime) (?)

The second point is a situation that cannot be created with synchronized, or locked uploads and needs extra attention.. And I think it might be a reason for the positions in comment:9 and comment:10.

[bastiK]

Please have a look at #15653! Apparently, there is an exception if the user deletes all layers while the upload is in progress.

[udit]

Apologies for the delay. I'll try working on it this week.

[udit]

The user could delete an OsmLayer while the background upload was in progress. I have added a check in the MainLayerManager class to check if the layer to be deleted is not being uploaded as well. The same has been done for the resetState function. I tested it locally.

[udit]

Bug fix for deletion

[Don-vip]

In 13213/josm:

fix #15653, see #8509 - IllegalStateException during Upload (regression from r13133 - patch by udit, minor changes)

[Don-vip]

Thanks! :)

[Don-vip]

In 13215/josm:

see #15653, see #8509 - add headless check for unit tests

[Don-vip]

In 13219/josm:

see #15653, see #8509 - fix unit test causing mayhem

[udit]

Hey don-vip, how do I check which unit-test is failing?

This is orthogonal to the ticket but if I run ant test it runs the entire test suite. My IDE setup with intelliJ is a bit broken so I am not sure which tests are problematic.

[Don-vip]

There is no magic trick. The easiest way is to fix your IDE settings so you can run a few unit tests manually. A good idea is to run all tests from the package where classes are modified.