Opened 18 years ago

Closed 17 years ago

#1662 closed defect (fixed)

Memory-Leaks with TomCat/Oracle/Connection-Pool

Reported by: umn-ms@… Owned by: unicoletti
Priority: high Milestone: 5.0 release
Component: Input - Native Oracle Spatial Support Version: unspecified
Severity: normal Keywords:
Cc: ivanopicco, fsimon

Description (last modified by dmorissette)

I made experiences with using Connection-Pooling of Oracle-Connections inside
Java/Tomcat.

Testenvironment: Mapserver 4.6.2; Suse-Linux; Tomcat 4.1.31; Sun-Java 1.4.2
Simulating 5 Browsers, which produce maps, query features, make selections,
query-legend-pics frequently.

- Big memory leak: Between the first 5 requests and the next 100 requests
   the Tomcat-process became about 400MB bigger.  (I use "top" for
   watching memory-footprint.)

- Cleaning the Connection-Pool "by hand". This means:
  Opening the function msConnPoolCloseUnreferenced in mappool.c
  for use in Java and call it after every request.

- After this I still have memory leaks:  About 100MB for 30.000 requests.
  (I also made a test: 25.000 requests without Connection pooling. Memory increased
    and decreased as expected  in this case.)

My Post on the list was:
http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=1661

The follow ups in the user-list are containing a small discussion on 
dealing this issue.

Attachments (8)

before-after.zip (64.3 KB ) - added by umn-ms@… 18 years ago.
Comparison: Before and after the proposed change (zip file, excel can be SLOW to open)
Memory.gif (11.7 KB ) - added by umn-ms@… 17 years ago.
Memory-footprint of testprogram
maporaclespatial.diff (8.4 KB ) - added by umn-ms@… 17 years ago.
Patch: Bug #1662 and #1961
benedikt-1662-revised.patch (4.5 KB ) - added by unicoletti 17 years ago.
Revised version of Benedikt's patch
maporaclespatial.c (95.3 KB ) - added by fsimon@… 17 years ago.
New maporaclespatial.c file
maporaclespatial.err.gz (8.2 KB ) - added by unicoletti 17 years ago.
Output of shp2img using Simon's patch
maporaclespatial.2.c (94.8 KB ) - added by fsimon@… 17 years ago.
New version of maporaclespatial.c
mapserver-maporaclespatial-memory_leak-ivano.patch (9.1 KB ) - added by ivanopicco 17 years ago.

Download all attachments as: .zip

Change History (54)

comment:1 by umn-ms@…, 18 years ago

Summary: Memory-LeaksMemory-Leaks with TomCat/Oracle/Connection-Pool

comment:2 by unicoletti, 18 years ago

Cc: unicoletti@… added

comment:3 by umn-ms@…, 18 years ago

bug_file_loc: http://mapserver.gis.umn.edu/bugs/show_bug.cgi?id=1661http://article.gmane.org/gmane.comp.gis.mapserver.user/16481

comment:4 by unicoletti, 18 years ago

I am investigating this issue using the unit test setup in bug #1664.

I have changed the test_connectionMaxLongTermBehaviour function so that it will
emulate 30000 registrations and releases and then ran the unit test under
valgrind. I have also commented all msConnPoolFinalCleanup calls as they force a
cleanup of all connections (effectively eliminating the memory leak issue).
In the real world msConnPoolFinalCleanup is never called in mapscript because it
should be called when the jvm exits, but at that point it is irrelevant whether
you clean up memory or not. For the record msConnPoolFinalCleanup is called in
msCleanup().

Now I am trying to understand whether this is what is happening to the user or
not (maybe a side-effect of the way unit tests are setup? I believe it is not,
but one can never be sure). I'll post updates here.

It would be useful if the user could reproduce the setup and call
msConnPoolFinalCleanup once in a while (every 100 requests for instance).

The valgrind report (limited to the memory leaks follows):

<snip>

==3554==
==3554== ERROR SUMMARY: 30000 errors from 17 contexts (suppressed: 43 from 1)
==3554== malloc/free: in use at exit: 1086808 bytes in 109 blocks.
==3554== malloc/free: 33192 allocs, 33083 frees, 1633108730 bytes allocated.
==3554== For counts of detected errors, rerun with: -v
==3554== searching for pointers to 109 not-freed blocks.
==3554== checked 1909892 bytes.
==3554==
==3554==
==3554== 4 bytes in 1 blocks are still reachable in loss record 1 of 4
==3554==    at 0x1B9005F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck.so)
==3554==    by 0x804D4C3: mappool_setup (mappool_tests.c:27)
==3554==    by 0x80B63F9: run_single_suite (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B5DCB: CU_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B6BF0: basic_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B69E0: CU_basic_run_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x804D421: main (main.c:18)
==3554==
==3554==
==3554== 512 bytes in 1 blocks are still reachable in loss record 2 of 4
==3554==    at 0x1B9005F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck.so)
==3554==    by 0x804D463: mappool_setup (mappool_tests.c:19)
==3554==    by 0x80B63F9: run_single_suite (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B5DCB: CU_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B6BF0: basic_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B69E0: CU_basic_run_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x804D421: main (main.c:18)
==3554==
==3554==
==3554== 2332 bytes in 106 blocks are still reachable in loss record 3 of 4
==3554==    at 0x1B9005F8: malloc (in /usr/lib/valgrind/vgpreload_memcheck.so)
==3554==    by 0x1BDD5B4F: strdup (in /lib/libc-2.3.3.so)
==3554==    by 0x804DEB7: msConnPoolRegister (mappool.c:272)
==3554==    by 0x804D69A: test_msConnPoolRegisterTwice (mappool_tests.c:64)
==3554==    by 0x80B65F1: run_single_test (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B6487: run_single_suite (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B5DCB: CU_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B6BF0: basic_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B69E0: CU_basic_run_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x804D421: main (main.c:18)
==3554==
==3554==
==3554== 1083960 bytes in 1 blocks are still reachable in loss record 4 of 4
==3554==    at 0x1B901116: realloc (in /usr/lib/valgrind/vgpreload_memcheck.so)
==3554==    by 0x804DE3C: msConnPoolRegister (mappool.c:253)
==3554==    by 0x804DB4E: test_connectionMaxLongTermBehaviour (mappool_tests.c:117)
==3554==    by 0x80B65F1: run_single_test (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B6487: run_single_suite (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B5DCB: CU_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B6BF0: basic_run_all_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x80B69E0: CU_basic_run_tests (in
/home/unicoletti/Workspace/eclipse/mapserver/cunit/cunit)
==3554==    by 0x804D421: main (main.c:18)
==3554==
==3554== LEAK SUMMARY:
==3554==    definitely lost: 0 bytes in 0 blocks.
==3554==      possibly lost: 0 bytes in 0 blocks.
==3554==    still reachable: 1086808 bytes in 109 blocks.
==3554==         suppressed: 0 bytes in 0 blocks.

comment:5 by unicoletti, 18 years ago

Owner: changed from fsimon@… to unicoletti
Benedikt,
could you give more information on these:

1) number of layers using oracle connections and total number of layers
2) how you disabled connection pooling exactly
3) in the last case ( 100MB increase for 30.000 requests ) could you say if the
amount of memory was always increasing over time or if it got to a point where
it did stabilize? In example: after 10000 requests it is 100MB and after 30000
it still is 100MB (I would expect this kind of behaviour from the current
connection pooling, btw) 

Simon, I reassign the bug to me.

Thanks,
Umberto

comment:6 by unicoletti, 18 years ago

Cc: fsimon@… added; unicoletti@… removed

comment:7 by umn-ms@…, 18 years ago

Hi Umberto

Thank you for taking care. Since my project is over it's not easy
for me to figure out details. (For example the computer, which runned the
testenvironment is easily not available anymore.)

> 1) number of layers using oracle connections and total number of layers
Mapfile contained 37 Oracle-Layers.
About half of them had "STATUS ON".
About 5 of theses contained noteworthy amount of data. (100-2000 rows)
Other contained testdata (1-5 rows) or no data.

> 2) how you disabled connection pooling exactly
I suppose this question refers to 
"I also made a test: 25.000 requests without Connection pooling"?
I deactivated by changing the Mapfile
  PROCESSING "CLOSE_CONNECTION=NORMAL"  or 
  #PROCESSING "CLOSE_CONNECTION=DEFER"

  I did *not* deactivated by mapscript
  I deactivated all Connection-Poolings in *all* layers. (Since
mapserver-Connection-Pooling
  seems to be "greedy" in a way)
  After changing the mapfile I restarted tomcat,
  
> 3) in the last case ( 100MB increase for 30.000 requests ) could you say if the
> amount of memory was always increasing over time or if it got to a point where
> it did stabilize? In example: after 10000 requests it is 100MB and after 30000
> it still is 100MB (I would expect this kind of behaviour from the current
> connection pooling, btw) 
I made a script to journalize space-amount over time. I made Excel-Plots
showing, that
memory-amount became continously bigger. In 12 hours it didn't stop to do so.


Some thought's:
----
I tried to look through your testprogram and the valgrind-output.
Do I understand right:
- It's enough to open Connections for a layer without doing something like drawing,
  to produce a leak? 
- The " msConnPoolRegister (mappool.c:253)"-message says, that
  the global static-variable "connections" is not freed porperly. Right?
  But it *is* freed in msConnPoolRelease. What happens?
----
If msConnPoolRegister is called 30000 times, the "connections"-array in
mappools.c increaes 
and is not shrinked. (Except it is shrinked to 0, when the array is empty). 
But in maporaclespatial.c it is taken care not to open a new connection for the
same layer.
Instead msConnPoolRequest is called to look for an existing connection. In this case
only the reference-count is increased and no new connectionObj is required.
----
Just a feeling: I believe the memory-leak rises in conjunction with
layer-handling (open,
close, draw, queryAttribute...). I steered on the mappols.c a lot. (bug 1402 :-)
I tend to trust it.
Obvioulsy I can mistake...


comment:8 by unicoletti, 18 years ago

>Thank you for taking care. Since my project is over it's not easy
>for me to figure out details. (For example the computer, which runned the
>testenvironment is easily not available anymore.)

That is a pity, as I do not have Oracle available too.

> > 1) number of layers using oracle connections and total number of layers
>Mapfile contained 37 Oracle-Layers.
>About half of them had "STATUS ON".
>About 5 of theses contained noteworthy amount of data. (100-2000 rows)
>Other contained testdata (1-5 rows) or no data.

ok.

> > 2) how you disabled connection pooling exactly
> I suppose this question refers to 
> "I also made a test: 25.000 requests without Connection pooling"?
>I deactivated by changing the Mapfile
>  PROCESSING "CLOSE_CONNECTION=NORMAL"  or 
>  #PROCESSING "CLOSE_CONNECTION=DEFER"
>
>  I did *not* deactivated by mapscript
>  I deactivated all Connection-Poolings in *all* layers. (Since
>mapserver-Connection-Pooling
>  seems to be "greedy" in a way)
>  After changing the mapfile I restarted tomcat,
  
I understand.

> I made a script to journalize space-amount over time. I made Excel-Plots
> showing, that
> memory-amount became continously bigger. In 12 hours it didn't stop to do so.

OK, so it is not connection pooling. It probably has to do with Oracle.
I only wanted to make sure so to restrict the amount of code to check.

> Some thought's:
> ----
> I tried to look through your testprogram and the valgrind-output.
> Do I understand right:
> - It's enough to open Connections for a layer without doing something like > >
drawing,
>   to produce a leak? 

No, it is not, at least that's what I also understood by looking at the valgrind
output.

> - The " msConnPoolRegister (mappool.c:253)"-message says, that
>  the global static-variable "connections" is not freed porperly. Right?

Correct.

>  But it *is* freed in msConnPoolRelease. What happens?

It is freed only if size is zero.
It is freed for sure in msConnPoolFinalCleanup. msConnPoolFinalCleanup is never
called in java mapscript as we have not found a hook yet.

> ----
> If msConnPoolRegister is called 30000 times, the "connections"-array in
> mappools.c increaes 
> and is not shrinked. (Except it is shrinked to 0, when the array is empty). 

Exactly. It will get large enough to hold 30000 connections.

> But in maporaclespatial.c it is taken care not to open a new connection for
> the
> same layer.
> Instead msConnPoolRequest is called to look for an existing connection. In
> this case
> only the reference-count is increased and no new connectionObj is required.

that depends: a new connections array needs to be opened if none is available in
the pool (for instance this happens when the first layer is drawn). So in theory
if there are many requests in a short amount of time the number of connections
can grow in a almost linear fashion (worst case, as if no pooling was available).

> Just a feeling: I believe the memory-leak rises in conjunction with
> layer-handling (open,
> close, draw, queryAttribute...). I steered on the mappols.c a lot. (bug 1402
> :-)
>I tend to trust it.
> Obvioulsy I can mistake...

I think you are right, but I wanted to make sure and get connection pooling out
of the suspects list.
Thanks for your patience.

comment:9 by umn-ms@…, 18 years ago

Ciao Umberto

I saw in the mailing-list, that you are insistent in solving this issue.

About testdata: I exported an 5.4MB zip-File with political boundaries and
waterbodies from our database. (political boundaries are polygons, 
waterbodies are lines with m-Values.)

Before importing this dump you must create a user WWI with enough db-grants.
Easiest way would be: 
CREATE USER WWI identfied by wwi;
Grant dba to wwi; 

My Oracle-Version is 9.2.0.5. So it shouldn't be a problem to import in 10.x.

There's also a small script for creating the proper spatial indices that must be
executed after export.

If you think it's helpfull, you may download it from 
http://www2.hydrotec.de/webdemos/be/oraexamples.zip 
(It will be there for some days.)

A working oracle-database-instance can be downloaded from 
http://www.vmware.com/vmtn/appliances/oracle.html

Best regards
Benedikt

by umn-ms@…, 18 years ago

Attachment: before-after.zip added

Comparison: Before and after the proposed change (zip file, excel can be SLOW to open)

comment:10 by umn-ms@…, 18 years ago

Hello Fernando and Umberto

I found some time to look for this issue. I think I can propose a codechange
which leads to a kind of solution.

- I wrote a simple Java-test-program with one loop. Inside the loop
  a webObj ist instantiated and a image is drawn.
  The corrsponding map-file simply consists of one Oracle-Spatial data-layer 
  with connection-pooling turned on.

- The memory leak appeares in this constellation. Therfore there is for now no
  more need to watch multithreading.

- Per map the leak increases with about 18200 KiloByte (!). (18 MB)
  
- Since the leak doesn't occure without pooling and the pooling itself seems to
  work correct, it looks like the memory-leak is made up of resources which
  are freed by OCI when the connection is closed.

- Because of this constellation and since the leak is very large I think 
    layer->layerinfo->obj is the source of the leak.
  It is allocated with OCIDefineObject by Oracle never freed.
 
http://www.utexas.edu/its/unix/reference/oracledocs/v92/B10501_01/appdev.920/a96584/oci15r36.htm
  states, that the memory which is allocated by OCIDefineObject must be freed
  with OCIObjectFree

- Fernando. I suppose, you know
 
http://www.oracle.com/technology/sample_code/products/spatial/files/oci_sample/ocicode.zip
  They free the generated objects with OCIObjectFree.
  
- I made some experiments with this and tried to implement this in
maporaclespatial.c.
  I didn't succeed.
  But I'm still quite sure, this is the right way.

- The codechange I propose is therfore quite inelegant. Oracle has a 
  function, which cleans all the stuff which is allocated in a connection
  without closing the connection: 
  
  In  msOracleSpatialLayerClose is a call to msConnPoolRelease:
      if (layer->debug)
        msDebug("msOracleSpatialLayerClose. Cleaning Oracle handlers.\n");
      msConnPoolRelease( layer, hand);
        
  Change this line to 
      if (layer->debug)
        msDebug("msOracleSpatialLayerClose. Cleaning Oracle handlers.\n");
      OCICacheFree (hand->envhp,hand->errhp,hand->svchp);
      msConnPoolRelease( layer, hand);

- Even in the case the leak can be handled more precisely later, I think this line
  makes sence to be prepared against many kinds of potential problems.
  
- I made some tests and put the results in an excel sheet.
  
  I measured the process-size every second.
  Therfore x-axes are approximaely seconds and y-axes is process-size of
  the java-test-program in kilobyte.
  
  Between each image-generation I paused the thread for a half second.
  
  The test shows:
  * The big memory-leak is fixed by the change
  * There is at least one more leaks which does not result from 
    not-freed-oracle-ressources.

- Althoug I'm unhappy with the kind of solution I think the change
  should be made, since it is worthfull for users.
  It's an advantage for all non-CGI-usage of mapserver.
  

comment:11 by unicoletti, 18 years ago

attachments.description: Comparison: Before and after the proposed changeComparison: Before and after the proposed change (zip file, excel can be SLOW to open)

comment:12 by unicoletti, 18 years ago

Status: newassigned
I can confirm that your patch alleviates the problem.
I have ran the threadtests target in mapscript/java on a map with an oracle
layer and I have seen memory usage drop down from 300M to 100M after I applied
the suggested modification.
This also confims that there still is another big memory leak somewhere
(accounting for part of the remaining 100M).

Great stuff Benedikt, thanks a lot!

comment:13 by fsimon@…, 18 years ago

Hi Benedikt and Umberto,
    I will check and test the solution here. I will check with all versions of
OCI (8i, 9i, 10G). After this I will fix and commit the changes in the
maporaclespatial.c
    Thanks Benedikt.
    Best regards.

comment:14 by unicoletti, 18 years ago

OCIDefineObject might be the cause for the other memory leaks. It appears to be
called at lines 1796, 2173 and 2342.
OCIObjectFree() is never called.

comment:15 by umn-ms@…, 18 years ago

Hi

code from msOracleSpatialLayerOpen may cause a leak:

     msOracleSpatialLayerInfo *layerinfo = alloc(..)
     msOracleSpatialDataHandler *dthand = alloc(..)
     ..
     if (layer->layerinfo != NULL) return

The allocs and the returns should be the other way round:
First the checks. Afterward allocate.

This one isn't harmfull in practice, since
calling layer::open on an open layer is unusual.

comment:16 by umn-ms@…, 18 years ago

> I have ran the threadtests target in mapscript/java on a map with an oracle
> layer and I have seen memory usage drop down from 300M to 100M after I applied
> the suggested modification.
Umberto

The testprogram MapThread leaves the cleaning of mapserver-resources, which are
held in the mapObj, to the finalizer.
So they are freed at the time webObj is freed by the garbage-collector.

The GC feels only responsible for the Java-objects. 
There are not many Java-Object instantiations. Hence there may be no need
for GC from the Java-GC point of view.
So the mapserver-resources may be freed very late.

- Which values did you give to -i and -t commandline parameters?
  So, how many webObj are instantiated?
- How much memory is needed, if map.delete() is called at the end of
  MapThread.run()

If you did set -t=1 these questions are dispensable.

BTW: I'm joyfully surprised, that this test passes anyway with -t>1

comment:17 by unicoletti, 18 years ago

> I have ran the threadtests target in mapscript/java on a map with an oracle
> > layer and I have seen memory usage drop down from 300M to 100M after I applied
> > the suggested modification.
> Umberto
> 
> The testprogram MapThread leaves the cleaning of mapserver-resources, which are
> held in the mapObj, to the finalizer.
> So they are freed at the time webObj is freed by the garbage-collector.
> 
> The GC feels only responsible for the Java-objects. 
> There are not many Java-Object instantiations. Hence there may be no need
> for GC from the Java-GC point of view.
> So the mapserver-resources may be freed very late.
> 

I am invoking System.gc() after every iteration and watching the gc log with
-Xloggc:/tmp/gc.log argument to the Java imterpreter.
The gc.log clearly shows that full gc are made quite often, so memory should
decrease accordingly, but it does increase instead.

> - Which values did you give to -i and -t commandline parameters?
>   So, how many webObj are instantiated?

-t 10 -i 5 (standard valies)

> - How much memory is needed, if map.delete() is called at the end of
>   MapThread.run()

I didn't try this, but I believe that it shouldn't make any difference.

> 
> If you did set -t=1 these questions are dispensable.
> 
> BTW: I'm joyfully surprised, that this test passes anyway with -t>1

The threadtest only aims at verifying thread safety, not memory leaks.

I'll try valgrind today.

comment:18 by fsimon@…, 18 years ago

Hi all,
    Can you attache your test script? I'm fizing this bug and I want to test
with yours scripts before commit the code in cvs tree.
    Best regards.

comment:19 by umn-ms@…, 17 years ago

Hi Fernando & Simon

Once again I found some time to look at this issue.

Please watch maporaclespatial.c line 1580 (version 4.10):
    msOracleSpatialHandler *hand = (msOracleSpatialHandler
*)malloc(sizeof(msOracleSpatialHandler));

    memset( hand, 0, sizeof(msOracleSpatialHandler) );

This memory is never used and never freed, since the msOracleSpatialHandler is
either
reused from the pool (line 1606) or allocated in msOCISetHandlers (called at
line 1611).

These two lines can be deleted, since they are useless.

After applying the patch I watched the memory-footprint of the Java-testprogram
again. During the creation of 2000 simple maps with one layer the program grew
less than 400 Byte (0.4 KByte).
I think this is satisfying.

Bye
Benedikt

PS: I found the leak by using http://www.linkdata.se/sourcecode.html,
included memwatch.h in map.h and wrote a simple testprogram in C:

#include <map.h>
#include <stdio.h>

#define runs 20

int main(int argc, char** argv) {
   int i;
   mapObj * m;
   imageObj* img;
   mwInit();
   msSetup();
   for (i = 0 ; i < runs; i++) {
      m = msLoadMap("U:\\ws\\mapserver\\test\\small.map", NULL);

      img =  msDrawQueryMap(m);
      msSaveImage(m, img, "U:\\ws\\mapserver\\test\\prg\\map.png");
      msFreeImage(img);

      msFreeMap(m);
      printf("Fertig mit Bild %d\n" , i);
   }
   mwTerm();
}


comment:20 by umn-ms@…, 17 years ago

UHPS! I made an awkward mistake :-(
The testprogram grew approximately 400 KByte (0.4 MByte).
More exactly: It grew from 43728KByte to 44160KByte.

Well, before the patch it lost ten times more. So the patch *is* good.
But now I'm not shure anymore, wether this is "satisfying".

I will make a bigger test an call the Garbage-Collector frequently to see 
wether this behaviour comes from Java (harmless) or from Mapserver(harmfull)



by umn-ms@…, 17 years ago

Attachment: Memory.gif added

Memory-footprint of testprogram

comment:21 by unicoletti, 17 years ago

Well done Benedikt!
Can you put together a patch against latest mapserver cvs?
Just ask if you need help...

Thanks,
Umberto

by umn-ms@…, 17 years ago

Attachment: maporaclespatial.diff added

Patch: Bug #1662 and #1961

comment:22 by unicoletti, 17 years ago

Benedikt
thanks for the patch.
I could apply it and it seems to work ok.
I'll take a look at memory usage ASAP, but I'm quite confident it will have
improved.

I am attaching a revised version of the patch: it seems that your patch
accounted also for all changes in tabs and whitespace which made it a bit
difficult to read (but it works and that's important).
Can you check if it's ok?

For a reference this is the command I used to generate the patch:
cvs diff -b -U 3 maporaclespatial.c > ../file.patch




by unicoletti, 17 years ago

Attachment: benedikt-1662-revised.patch added

Revised version of Benedikt's patch

comment:23 by unicoletti, 17 years ago

attachments.isobsolete: 01

comment:24 by fsimon@…, 17 years ago

Hi folks,
    I will apply this patch this morning, I will test here and post the results.
    I will test with the script that you sent in the comment #15, this is the
last version of your test program? Umberto, do you have another too?
    Best regards.

comment:25 by unicoletti, 17 years ago

I used shp2img, one of the Java examples (DrawMap.java) and Java threadtest to
check the patch.

by fsimon@…, 17 years ago

Attachment: maporaclespatial.c added

New maporaclespatial.c file

comment:26 by fsimon@…, 17 years ago

attachments.description: New codeNew maporaclespatial.c file

comment:27 by umn-ms@…, 17 years ago

Hi Fernando

Actually I don't have time for real testing. I just watched the code a 
little bit:
----------------------------------------------------------------------
...
        msConnPoolRelease( layer, layerinfo->orahandlers );
...
        OCICacheFree
(layerinfo->orahandlers->envhp,layerinfo->orahandlers->errhp,layerinfo->orahandlers->svchp);

By doing so OCICacheFree will be called on a closed and freed connection. That
is to say: The pointers given to OCICacheFree are not valid anymore.

Maybe in many cases it won't crash, but I think this is hazardous and suggested
therefore to change the order of the free-sequence in msOracleSpatialLayerClose. 

----------------------------------------------------------------------
In msOracleSpatialLayerWhichShapes you decided to allocate "items" with malloc
instead of alloca. In this case it must be freed. As fare as I can see, this
doesn't happen.
Same in msOracleSpatialLayerGetExtent.


By
Benedikt

comment:28 by unicoletti, 17 years ago

Simon,
I have tried your patch, but is seems to report an error (see attachment).

I have run shp2img to test the patch.

by unicoletti, 17 years ago

Attachment: maporaclespatial.err.gz added

Output of shp2img using Simon's patch

comment:29 by umn-ms@…, 17 years ago

Hi Umberto

Problably this information is superfluous or deprecated now. Concerning your
"patched patch":
1. Sorry: I forgot to give "-b" in diff. I did'nt want to mix up whitespaces
2. I checke dthe patch. It's correct.

Benedikt

by fsimon@…, 17 years ago

Attachment: maporaclespatial.2.c added

New version of maporaclespatial.c

comment:30 by fsimon@…, 17 years ago

attachments.isobsolete: 01

comment:31 by fsimon@…, 17 years ago

Hi folks,
    Did you already test with this new version? 
    I need to check this bug before change some parts of the source code. I need
to close another Oracle bugs :)
    Best regards from Brazil.
   

comment:32 by unicoletti, 17 years ago

I haven't tested, I'll try next week (I'm busy working on rfc24).
Shall I apply against latest cvs?

comment:33 by bene, 17 years ago

Hi Fernando

Concerning maporaclespatial.2.c: In msOracleSpatialLayerGetShape the nullind-memory is not freed.

Benedikt

comment:34 by fsimon, 17 years ago

Hi folks, I'm going back now. Some time between the semesters in Master. Can we improve the things to solve this issue before the 5.0 release? I will be fixing the code about the last issue. Best regards.

comment:35 by unicoletti, 17 years ago

I suggest that we start from comment 24 (the revised version of Benedikt's original patch): it still applies successfully to the latest svn. This definitely needs to be closed for 5.0.

comment:36 by bene, 17 years ago

Hi Fernando and Umberto

It would be great to have this in in 5.0!

Unfortunately I have no time to make tests with the patches from Fernando, since I'm involved in other projects (and hollidays).

The patch from "11/21/06 03:11:15 changed by unicoletti" is in use at some of our customer-installations. I often talk with them, but I didn't hear from any problems. So look's like it works fine.

I'd be fine with Umberto's suggestion

Benedikt

comment:37 by ivanopicco, 17 years ago

Hi all, I made some test and i don't found any problems. I've used a revised patch with nullind free. My patch is attached. Hope this helps. Best regards. Ivano

comment:38 by fsimon, 17 years ago

Hi all,

I will check the code and add other things (I have another bug's to fix) and after commit. Just a question Ivano, what version did you patched? The code from comment 30/31? This fix will be in 5.0 release.

comment:39 by ivanopicco, 17 years ago

Hi all, I started from maporaclespatial.2.c and I merged it with free(nullind).

comment:40 by ivanopicco, 17 years ago

Cc: ivanopicco added; fsimon@… removed

comment:41 by fsimon, 17 years ago

The code will be commit this afternoon in the SVN.

comment:42 by fsimon, 17 years ago

Cc: fsimon added

comment:43 by fsimon, 17 years ago

I committed the code in the SVN this afternoon. The revision is 6435. Someone can test this release before close the bug? Best regards.

comment:44 by dmorissette, 17 years ago

Description: modified (diff)
Milestone: 5.0 release

comment:45 by ivanopicco, 17 years ago

Tested code from revision r6441. It works form me, wrt this bug. Performance issue (bug #2179) is still here. Best regards.

comment:46 by dmorissette, 17 years ago

Resolution: fixed
Status: assignedclosed

I believe we can mark this fixed now?

Note: See TracTickets for help on using tickets.