proj_create_operations intermittent failures #1736
Assignees
Comments
|
Trying to reproduce... |
|
@nyalldawson I'm unfortunately unsuccessful to reproduce. I've create the below test code from your indications, and added threading as well just in case. The "Unimplemented" error you get on the last 2 operations is expected given that EPSG lists 2 grids unknown of PROJ #include "proj.h"
#include <assert.h>
#include <stdio.h>
#include <thread>
void f(PJ_CONTEXT *c)
{
PJ_OPERATION_FACTORY_CONTEXT *operationContext = proj_create_operation_factory_context( c, nullptr );
proj_operation_factory_context_set_grid_availability_use( c, operationContext, PROJ_GRID_AVAILABILITY_IGNORED );
proj_operation_factory_context_set_spatial_criterion( c, operationContext, PROJ_SPATIAL_CRITERION_PARTIAL_INTERSECTION );
PJ *src = proj_create_from_database( c, "EPSG", "3035", PJ_CATEGORY_CRS, false, nullptr );
PJ *dest = proj_create_from_database( c, "EPSG", "5514", PJ_CATEGORY_CRS, false, nullptr );
//PJ *src = proj_create_from_database( c, "EPSG", "3059", PJ_CATEGORY_CRS, false, nullptr );
//PJ *dest = proj_create_from_database( c, "EPSG", "4326", PJ_CATEGORY_CRS, false, nullptr );
//PJ *src = proj_create_from_database( c, "EPSG", "3908", PJ_CATEGORY_CRS, false, nullptr );
//PJ *dest = proj_create_from_database( c, "EPSG", "3908", PJ_CATEGORY_CRS, false, nullptr );
if ( PJ_OBJ_LIST *ops = proj_create_operations( c, src, dest, operationContext ) )
{
int count = proj_list_get_count(ops);
for(int i = 0; i < count; i++ )
{
PJ* op = proj_list_get(c, ops, i);
const char* proj_str = proj_as_proj_string(c, op, PJ_PROJ_5, nullptr);
//printf("%d: %s\n", i, proj_str ? proj_str : "null_result");
proj_destroy(op);
}
proj_list_destroy(ops);
}
else
{
// BAD!
assert( false );
}
proj_destroy(src);
proj_destroy(dest);
proj_operation_factory_context_destroy(operationContext);
}
void thread_function()
{
PJ_CONTEXT *c = proj_context_create();
while(1)
f(c);
}
int main()
{
std::thread t1(thread_function);
std::thread t2(thread_function);
t1.join();
t2.join();
return 0;
}But... from the errors mentionned in the QGIS tickets, I have suspected some black-magic inside PROJ. PROJ C++ geodetic-related objects (CRS, CoordinateOperation, etc..) are immutable... Mostly... That is: user code normally cannot mutate them by using public methods, but internal PROJ code in some parts for complex reasons has to mutate them sometimes (using protected methods and friend class to limit the callers). Actually this is limited to hacking the source and target CRS of CoordinateOperation objects, for the purpose of chaining them together in a ConcatenatedOperation. Where this could fail is if it mutates objects that are cached (the DatabaseContext c++ object that is attached to a PJ_CONTEXT has a cache of direct coordinate operations for CRS A -> CRS B transformations). Normally PROJ takes measures to create a "shallow clone" before mutating objects that could potentially be cached, so that the cached objects are not mutated. But of course there might be code paths where this has been overlooked... I've added an extra debug sanity check in https://github.com/rouault/PROJ/tree/6.2.1_with_immutable_flag_checker which is plain 6.2.1 with rouault@264b0b5 applied on top of it. The added assertion doesn't fail when running the PROJ autotest suite or the above reproducer attempt, but QGIS use cases might trigger other situations, so perhaps try to run QGIS with this instrumented version |
|
Thanks @rouault , I'll test with that. Fwiw I don't think threading is an issue here - I've been able to reproduce with single thread access alone. |
How do you reproduce with QGIS ? I tried the project attached in qgis/QGIS#30569 (comment) but works fine for me with a QGIS master from 2 weeks ago or so and PROJ master (non instrumented) |
|
rouault@264b0b5 applies cleanly on top of PROJ master as well. I've just given it a try with the project attached in qgis/QGIS#30569 (comment) and the assert doesn't trigger |
|
update: I've identified a related issue, in GDAL, with a proposed fix in OSGeo/gdal#2047. I'm not sure that this fix actually fixes the issue people see in practice in QGIS... |
rouault
added a commit
that referenced
this issue
Nov 24, 2019
…ith overriding CRS on a InverseCoordinateOperation (could be related to #1736)
|
@nyalldawson You might want to try very latest PROJ master (or apply 875aba7 on top of 6.2.1). I couldn't raise this issue with current code however, which doesn't mean that this couldn't be hit... but found that while working on optimizations. |
|
Will test with master shortly. In the meantime, here's a full backtrace from the point the exception is raised: |
|
No luck with master -- I still see the issue |
:-( Do you have a reproducer that you could share ? |
|
@rouault check your email |
rouault
added a commit
to rouault/PROJ
that referenced
this issue
Nov 25, 2019
…CRS: do not mess the derivingConversion object of the original object (fixes OSGeo#1736) normalizeForVisualization(), promoteTo3D(), demoteTo2D(), alterGeodeticCRS(), alterCSLinearUnit() and alterParametersLinearUnit() all used the object returned by derivingConversionRef() to create a new ProjectedCRS. While doing so, this caused the derivingConversion of the original object to have its targetCRS set to the object returned by normalizeForVisualization() and similar. If that object died, then the weak pointer would be reset, and the original ProjectedCRS() has now its derivingConversionRef()->targetCRS() nullptr So bottom line is use derivingConversion() for anything that is not pure reading !!! This is confirmed to be the fix for the QGIS scenario in qgis/QGIS#30569 (comment) In QGIS use case, the issue arised when using a projected CRS with a non-GIS friendly axis (that is where normalizeForVisualization() created a new projectedCRS)
rouault
added a commit
to rouault/PROJ
that referenced
this issue
Nov 25, 2019
…CRS: do not mess the derivingConversion object of the original object (fixes OSGeo#1736) normalizeForVisualization(), promoteTo3D(), demoteTo2D(), alterGeodeticCRS(), alterCSLinearUnit() and alterParametersLinearUnit() all used the object returned by derivingConversionRef() to create a new ProjectedCRS. While doing so, this caused the derivingConversion of the original object to have its targetCRS set to the object returned by normalizeForVisualization() and similar. If that object died, then the weak pointer would be reset, and the original ProjectedCRS() has now its derivingConversionRef()->targetCRS() nullptr So bottom line is use derivingConversion() for anything that is not pure reading !!! This is confirmed to be the fix for the QGIS scenario in qgis/QGIS#30569 (comment) In QGIS use case, the issue arised when using a projected CRS with a non-GIS friendly axis (that is where normalizeForVisualization() created a new projectedCRS)
|
@nyalldawson Good news !!! I've a fix ready in #1746 @kbevers Once the above PR passes fine for master, I'll backport it in the 6.2 branch. We should consider if we issue a urgent 6.2.2 release. Or perhaps the QGIS project will be fine to build from the HEAD of 6.2 branch or cherry-pick the patch for OSGeo4W purposes, but this also affects all platforms. So a 6.2.2 is probably required in a not so distant future. |
As far as I can tell the next QGIS release is scheduled for the 29th of November. I am not sure I can find the time to fit in a PROJ release before that. I can probably make it for a December 1st release but I don't know if that would be useful for the QGIS guys. |
rouault
added a commit
that referenced
this issue
Nov 25, 2019
normalizeForVisualization() and other methods applying on a ProjectedCRS: do not mess the derivingConversion object of the original object (fixes #1736)
rouault
added a commit
that referenced
this issue
Nov 25, 2019
…CRS: do not mess the derivingConversion object of the original object (fixes #1736) normalizeForVisualization(), promoteTo3D(), demoteTo2D(), alterGeodeticCRS(), alterCSLinearUnit() and alterParametersLinearUnit() all used the object returned by derivingConversionRef() to create a new ProjectedCRS. While doing so, this caused the derivingConversion of the original object to have its targetCRS set to the object returned by normalizeForVisualization() and similar. If that object died, then the weak pointer would be reset, and the original ProjectedCRS() has now its derivingConversionRef()->targetCRS() nullptr So bottom line is use derivingConversion() for anything that is not pure reading !!! This is confirmed to be the fix for the QGIS scenario in qgis/QGIS#30569 (comment) In QGIS use case, the issue arised when using a projected CRS with a non-GIS friendly axis (that is where normalizeForVisualization() created a new projectedCRS)
|
Pull request fixing in master merged, and cherry-picked in 6.2 branch per bce4b15 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I've struggled to narrow down the situation which triggers this, but unfortunately this is the best I can get....
Given the following code:
I hit random behavior where sometimes this fails, and other times it correctly returns a handful of results.
From what I've been able to debug:
If I use a brand new context whenever I call this (
PJ_CONTEXT *c = proj_context_create();) then there's never an issue, and the operations are always returned.If I use an existing context, or a nullptr for the context, then sometimes the call fails with the error
internal_proj_create_operations: At least one of the operation lacks a source and/or target CRSIt seems like some other operation performed using the context prior to this call is leaving it in an inconsistent state.
Although I can't 100% confirm it, I think this is caused by the following sequence of events:
Using a context, use proj_create_operations (as above) to get all operations between -s EPSG:3035 -t EPSG:5514.
iterate through these operations using proj_list_get. For each, call proj_as_proj_string.
Some of the operations fail to export to proj, with the error: "Unimplemented" (maybe due to missing grids locally?)
Using the SAME context, try to repeat the call to proj_create_operations at some later stage -- the operation fails
This issue is the underlying cause of qgis/QGIS#30569 (comment)
The text was updated successfully, but these errors were encountered: