for SQlite vectors wait rendering end to avoid file lock: fixes #15498 #5305

luipir · 2017-10-05T16:06:45Z

Description

In case of SQLite based vectors as Spatialite or GPKG, before to saveEdits, wait for end of rendering to avoid error of "File lock". The user is notified of the deferred save via messageBar. the message is automatically removed after rendering is terminated.

Checklist

Reviewing is a process done by project maintainers, mostly on a volunteer basis. We try to keep the overhead as small as possible and appreciate if you help us to do so by completing the following items. Feel free to ask in a comment if you have troubles with any of them.

Commit messages are descriptive and explain the rationale for changes
Commits which fix bugs include fixes #11111 in the commit message next to the description
Commits which add new features are tagged with [FEATURE] in the commit message
Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and containt sufficient information in the commit message to be documented
I have read the QGIS Coding Standards and this PR complies with them
This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
New unit tests have been added for core changes
I have run the scripts/prepare-commit.sh script before each commit

…qgis#15498

rouault · 2017-10-05T16:30:01Z

src/app/qgisapp.cpp

+  // Wait only if the current layer is in the rendering pool
+  if ( mMapCanvas->mapSettings().layers().contains( vlayer->id() ) )
+  {
+    if ( vlayer->dataProvider()->storageType().count( "SQLite" ) )


Either the above comment or this test are incorrect. The GPKG driver will identify itself as GPKG and not SQLite. Actually perhaps the issue doesn't occur with GPKG since we turn WAL on, and normally in WAL mode, readers and writers shouldn't conflict.
And shouldn't that also affect the QGIS own spatialite provider ?

sorry for my late, I was sick... now checking the correct storageType returned by:

plain SQlite

spatialite (I remeber is "SQLite with spatialite extension")

gpkg

plain SQLite => "SQlite"

external splite => "SQlite"

empty splite created in qgis => "SQLite database with SpatiaLite extension"

demo gpkgs => "GPKG"
=> adapting PR

luipir · 2017-10-10T15:41:32Z

my only concerns about the solution I proposed is that the message sent to the user is done using QgsMessageBar. MessageBar, changing the extent of the drawing canvas, trigger a refresh appearing and disappearing.
Would have been beter to think messagebar as stacked widget with canvas in a same layout. In this way any message does not trigger repaing... but this is out of scope of this PR

dakcarto · 2017-10-10T20:36:07Z

@luipir wrote:

my only concerns about the solution I proposed is that the message sent to the user is done using QgsMessageBar. MessageBar, changing the extent of the drawing canvas, trigger a refresh appearing and disappearing.
Would have been beter to think messagebar as stacked widget with canvas in a same layout. In this way any message does not trigger repaing... but this is out of scope of this PR

QgsMessageBar doesn't trigger a canvas redraw event, i.e. it is a layered/stacked widget already.

Test with the following in Python console, with layers loaded:

iface.messageBar().pushMessage("Hey","there")

dakcarto · 2017-10-10T20:42:39Z

src/app/qgisapp.cpp

+        QgsMessageBarItem *item = new QgsMessageBarItem( tr( "Save edit paused" ),
+            tr( "Waiting for end of rendering" ),
+            QgsMessageBar::WARNING, 0 );
+        messageBar()->pushItem( item );


Probably just use messageBar()->pushWarning(...) instead of creating a new object, since the widget doesn't have any custom child widgets.

or messageBar()->pushMessage(...) if you want no duration.

Nevermind. Sorry, should have reviewed all the code first. I see you reference QgsMessageBarItem object below.

exactly, I need the item reference to remove it directly after save commit is done

If I get this right, you are basically adding a sleep in the main thread (that will block the GUI, even if it's for a short amount of time), have you considered adding a singleshot timer that re-triggers the same slot in case the canvas is drawing? This would have the same effect (delaying the commit) but without blocking the GUI.

For master and C++11, a lambda would probably be the best way to implement it (see snippet below), but there might be ways to do that in 2.x too.

QTimer::singleShot(100, this, [ = ] { saveEdits( layer, leaveEditable, triggerRepaint ); });

Also, instead of hardcoding the storage type, I would suggest to add a capability flag to the data provider class that indicates if there support for file lock (or the lack of it).

Another idea: what about connecting to connect(mMapCanvas, &QgsMapCanvas::renderComplete ?

I though about this kind of solution, but the main problem to introduce async management is that it would change/brake to "save editing" feature execution flow. In my implementation I just introduce a kind of not blocking "sleep" instead of restructuring the set of method calls.
Another reason is to be conservative in bug fixing mode to avoid to introduce modification in the sequence of calls that can introduce more side effects (or regressions). Side effect not checked by unit or integrations tests because lack of them in the 2.x branch.

About adding a capability, I also though about this, but I was not conformtable to enter a kind a capability that depend make sense only in some rendering cases. A Capability should be an objective property of a provider, so how to call it? isLockedDuringAccess() isLockedConcurrentwriteReadAccess() ? Because wasn't clear how to define this capability I preferred to avoid to introduce a new capability for only a bug fix.
I'm open to find a general capability semanttic that can fit this provider case.

A delay/sleep in the main thread is blocking by definition and it's not very robust, because you'll never know if the delay is long enough.

to use the main event loop I can single shooting QgisApp::saveEdits slot instead of creating a new event loop.
The main idea that I want to maintain is to avoid to change the current code flow. Tha is exactly the solutuion proposed by Alessandro.

dakcarto · 2017-10-10T23:23:40Z

src/app/qgisapp.cpp

+        // wait
+        while ( mMapCanvas->isDrawing() )
+        {
+          QgsApplication::instance()->processEvents( QEventLoop::AllEvents, 100 );


Is this just to sleep for 100 ms, or do the app's events specifically need processed?

This also worked for me and also did not block the GUI:

QEventLoop loop; QTimer t; connect( &t, SIGNAL( timeout() ), &loop, SLOT( quit() ) ); t.start( 100 ); loop.exec();

@elpaso, was your concern the usage of QgsApplication::instance()->processEvents()? If so, does this handle it more appropriately, and if not, what do you suggest?

is for sleep without blocking, events have to be processed without blocking user interaction with the interface.
The proposed solution use the common and unique event loop.
I can't say if there are drawback on using QgsApplication::instance()->processEvents.

BTW I understand it's better an isolated event loop for an isolated feature than interact with common used event loop possibly interfering with other parts of the code.

luipir · 2017-10-11T14:00:54Z

PyQgsComposerPicture failed test, in my experience is a random failing test. is it possibile to retrigger build?

luipir · 2017-10-11T15:34:11Z

@elpaso @dakcarto simplified stopping rendering just before commitChanges

dakcarto · 2017-10-11T15:59:44Z

@luipir, latest changes work smoothly here on macOS 10.11.6!

luipir · 2017-10-11T16:27:30Z

now PyQgsMapLayerRegistry is failing and not PyQgsComposerPicture :(. But it locally passes :/

in case SQlite vectors wait end of rendering to avoid file lock: fixes …

8364b8d

…qgis#15498

rouault reviewed Oct 5, 2017

View reviewed changes

correct recognition of GPKG during save commit

8b8d085

dakcarto reviewed Oct 10, 2017

View reviewed changes

used independent QEventLoop instead of processEvents

46551a9

simplified stopRendering just before commitChanges

206965c

dakcarto approved these changes Oct 11, 2017

View reviewed changes

elpaso approved these changes Oct 11, 2017

View reviewed changes

elpaso merged commit 6321923 into qgis:release-2_18 Oct 12, 2017

luipir deleted the issue_15498_waitRenederingEndToAvoidLock branch October 13, 2017 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

for SQlite vectors wait rendering end to avoid file lock: fixes #15498 #5305

for SQlite vectors wait rendering end to avoid file lock: fixes #15498 #5305

luipir commented Oct 5, 2017 •

edited

Loading

rouault Oct 5, 2017

luipir Oct 9, 2017

luipir Oct 9, 2017 •

edited

Loading

luipir commented Oct 10, 2017

dakcarto commented Oct 10, 2017

dakcarto Oct 10, 2017

dakcarto Oct 10, 2017

dakcarto Oct 10, 2017

luipir Oct 11, 2017

elpaso Oct 11, 2017

elpaso Oct 11, 2017

luipir Oct 11, 2017

luipir Oct 11, 2017

elpaso Oct 11, 2017

luipir Oct 11, 2017 •

edited

Loading

dakcarto Oct 10, 2017

luipir Oct 11, 2017

luipir Oct 11, 2017

luipir commented Oct 11, 2017

luipir commented Oct 11, 2017

dakcarto commented Oct 11, 2017

luipir commented Oct 11, 2017

for SQlite vectors wait rendering end to avoid file lock: fixes #15498 #5305

for SQlite vectors wait rendering end to avoid file lock: fixes #15498 #5305

Conversation

luipir commented Oct 5, 2017 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

luipir commented Oct 10, 2017

dakcarto commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luipir commented Oct 11, 2017

luipir commented Oct 11, 2017

dakcarto commented Oct 11, 2017

luipir commented Oct 11, 2017

luipir commented Oct 5, 2017 •

edited

Loading

luipir Oct 9, 2017 •

edited

Loading

luipir Oct 11, 2017 •

edited

Loading