Opened 11 years ago

Closed 4 years ago

#560 closed defect (fixed)

WinGRASS not deleting temp ppm files from map display

Reported by: isaacullah Owned by: grass-dev@…
Priority: critical Milestone: 6.4.4
Component: Display Version: 6.4.0 RCs
Keywords: v.digit, ppm, temp, wingrass Cc: martinl
CPU: x86-32 Platform: MSWindows XP

Description

When v.digit loads, it creates it's own ppm and ppg files for the background of the v.digit display (seperate from the ppm and ppg files created for the regular tcltk map display). When one exits from v.digit, those ppm and ppg stay in the .tmp directory of the mapset. When one exits from grass, the ppm and ppg files for the map display are erased, but NOT those created by the v.digit display. This leads to build up of files that can eat harddrive memory. This problem was dectected on a Windows XP machine, and has not been tested for on Linux or Mac.

Change History (35)

comment:1 Changed 11 years ago by hamish

the only thing I can think of is that the files are still in use, so can not be swept up by clean_temp on exit. ?

maybe remove the "> NUL:" from the end of the 3 calls to it in init.bat,

rem Clean out old .tmp files from the mapset
"%WINGISBASE%\etc\clean_temp" > NUL:

and see if it produces an error message.

Hamish

comment:2 in reply to:  1 Changed 11 years ago by glynn

Replying to hamish:

the only thing I can think of is that the files are still in use, so can not be swept up by clean_temp on exit. ?

AFAICT, clean_temp doesn't work (at all) on Windows. See the "TODO"s in the code.

clean_temp only removes files if they are owned by the current user, and either the PID is no longer in use or the file is more than 4 days old.

But it cannot determine the current user on Windows (no getuid()), so it uses -1, but MSVCRT's stat() always reports zero for the st_uid field. Also, the code which checks whether a given PID is still in use always fails (which is handled the same as if the PID *is* in use).

If the current user check was skipped, it should at least delete files more than 4 days old.

Implementing the ownership check would be non-trivial, due to the complexity of the NT security model (e.g. a file can be owned by a group rather than a user, there's a difference between owner and creator, a "user" consists of both a domain and a username, etc).

comment:3 Changed 10 years ago by hamish

Keywords: wingrass added

comment:4 Changed 9 years ago by hamish

Hi,

any reason that GUI .ppm image layers are written to /tmp/ (or %TEMP%) and not /tmp/grass6-$USER-$PID ?

that would at least help mitigate the problem.

Hamish

comment:5 Changed 9 years ago by hellik

Milestone: 6.4.06.4.2

comment:6 Changed 8 years ago by hamish

wxgui layer manager is still leaving .pgm and .ppm files behind in C:\Documents and Settings\$USER\Local Settings\Temp

with time this folder grows large with orphaned files.

Hamish

comment:7 Changed 8 years ago by martinl

Cc: martinl added

comment:8 Changed 8 years ago by hamish

Priority: majorcritical

this isn't just v.digit; just using the wxGUI layer manager to render raster and vector maps is enough to leave many tmp*.ppm and tmp*.pgm files behind.

upgrading to critical because over time many MB of .ppm files will pile up in C:\Documents and Settings\$USER\Local Settings\Temp and eat all your disk space.

Hamish

comment:9 in reply to:  8 ; Changed 7 years ago by hellik

Replying to hamish:

this isn't just v.digit; just using the wxGUI layer manager to render raster and vector maps is enough to leave many tmp*.ppm and tmp*.pgm files behind.

upgrading to critical because over time many MB of .ppm files will pile up in C:\Documents and Settings\$USER\Local Settings\Temp and eat all your disk space.

in Win7 the left files are in C:\Users\$USER\AppData?\Local\Temp

Helmut

comment:10 in reply to:  9 ; Changed 7 years ago by martinl

Replying to hellik:

Replying to hamish:

this isn't just v.digit; just using the wxGUI layer manager to render raster and vector maps is enough to leave many tmp*.ppm and tmp*.pgm files behind.

upgrading to critical because over time many MB of .ppm files will pile up in C:\Documents and Settings\$USER\Local Settings\Temp and eat all your disk space.

in Win7 the left files are in C:\Users\$USER\AppData?\Local\Temp

These files should be stored in .tmp directory localed in current GRASS mapset I would say.

comment:11 in reply to:  10 ; Changed 7 years ago by glynn

Replying to martinl:

These files should be stored in .tmp directory localed in current GRASS mapset I would say.

The the mapset's .tmp directory is meant for files which will be moved to elsewhere within the mapset directory once they have been written (e.g. the cell/fcell files). Files cannot be rename()d across filesystem (partition) boundaries, so the temporary file must be on the same filesystem as the intended destination. The mapset's .tmp directory can be assumed to be on the same filesystem as the rest of the mapset.

Files which don't have this requirement should either use the OS' temporary directory, or a configurable directory. Access to the temporary directory may be significantly faster than to the mapset directory, particularly if the mapset is on a network filesystem.

comment:12 in reply to:  11 ; Changed 7 years ago by wenzeslaus

Replying to glynn:

Files which don't have this requirement should either use the OS' temporary directory, or a configurable directory. Access to the temporary directory may be significantly faster than to the mapset directory, particularly if the mapset is on a network filesystem.

So, all files created for wxGUI are standard temporary files without special requirements and should be created by standard system/python way, is that rigth?

Standard way is using tempfile.mkstemp and then delete the file (and of course, this has to be ensured even when error/exception occurs). Files which was not opened (files to be combined to resulting map) doesn't have to be closed. Only files which was opened (like resulting map) is necessary to close (see this).

Now, mkstemp is used for files which will be combined to resulting and then os.remove (grass.try_remove) is called. So, I'm not sure where is the problem (why temporary files are not removed).

comment:13 in reply to:  12 ; Changed 7 years ago by glynn

Replying to wenzeslaus:

So, all files created for wxGUI are standard temporary files without special requirements and should be created by standard system/python way, is that rigth?

AFAIK, yes.

Files which was not opened (files to be combined to resulting map) doesn't have to be closed.

mkstemp returns an OS-level handle to the opened file. This should be closed (with os.close()) if it is not required.

Now, mkstemp is used for files which will be combined to resulting and then os.remove (grass.try_remove) is called. So, I'm not sure where is the problem (why temporary files are not removed).

On Windows, you can't remove a file if any process has it open.

comment:14 in reply to:  13 ; Changed 7 years ago by neteler

Replying to glynn:

Replying to wenzeslaus:

...

Now, mkstemp is used for files which will be combined to resulting and then os.remove (grass.try_remove) is called. So, I'm not sure where is the problem (why temporary files are not removed).

On Windows, you can't remove a file if any process has it open.

Could the removal be put into the shutdown sequence? Perhaps by putting the files in question into a (file based) list?

comment:15 in reply to:  14 Changed 7 years ago by neteler

Milestone: 6.4.26.4.3

Replying to neteler:

Could the removal be put into the shutdown sequence? Perhaps by putting the files in question into a (file based) list?

Any opinion on this? Or delete at startup?

comment:16 Changed 7 years ago by neteler

See also #1286 (clean_temp can not be called before LOCATION_NAME is set)

comment:17 Changed 7 years ago by hamish

just tested, it's still an issue.

the gui should be cleaning up after itself, but perhaps the wxGUI temp .ppm, .pgm, and 0 byte no-extension versions of the same files could at least be placed in a \grass-wxgui subdir of \Temp\, so at least they are consolidated and easier to manually bulk clean up if grass is shutdown uncleanly?

Hamish

comment:18 Changed 7 years ago by hamish

fwiw the tcl/tk gui in wingrass saves its .ppm files to $MAPSET/.tmp, and succeeds in cleaning them up. It uses g.pnmcomp too, so I guess that g.pnmcomp exiting without explicitly closing the image files is not the reason for failure-to-delete?

Hamish

(argh trac is logging me off every 10 minutes today..)

comment:19 Changed 7 years ago by hamish

the empty file versions may be related to #943, core/render.py creates some temp files without extension, then creates some more in /tmp/ with e.g. .ppm extension. Note that way cancels the benefit of using tempfile.mkstemp(), since the temp file created is no longer created in a secure way, and /tmp is a+w. (at least $MAPSET/.tmp/ is u+w only)

after the empty temp files, there are still .ppm files not being removed too.

Hamish

comment:20 Changed 6 years ago by hamish

some slight improvement in devbr6 with r56444, but still more to do.

is there a reason for the temp .ppm files not to live in /tmp/grass6-$USER-$PID/ with gisrc for easier cleanup if something crashes or MS Windows happens? If the files are left open maybe it means that the gisrc temp dirs don't get deleted on MS Windows then..?

Hamish

comment:21 in reply to:  20 Changed 6 years ago by glynn

Replying to hamish:

some slight improvement in devbr6 with r56444, but still more to do.

is there a reason for the temp .ppm files not to live in /tmp/grass6-$USER-$PID/ with gisrc for easier cleanup if something crashes or MS Windows happens?

The location of the temporary directory is chosen by the init script, which doesn't "publish" the location directly. If GRASS was started using the init script, the directory can be inferred from $GISRC, but there's no reliable way to know whether GRASS was actually started using the init script.

Most uses of G_tempfile() are wrong (including its use by g.tempfile), but there isn't an official API for obtaining a normal temporary directory. AFAICT, the only code which should actually be using G_tempfile() is lib/raster/open.c and lib/raster3d/open.c.

In light of that, G_tempfile() should probably be changed to use a similar mechanism to the init script (possibly by having the script set GRASS_TMPDIR), and a new function created for the code which requires the existing behaviour (i.e. requires that the temporary file is within the mapset directory so that rename() and link() work).

comment:22 Changed 6 years ago by hamish

Ok, if $GISRC is custom you'd get tmp files in weird places. (render.py could test if the getenv string started with "/tmp/grassX-" before using that, otherwise use $TMPDIR?)

Right or wrong, many modules leave left-over files in $MAPSET/.tmp/, either due to design or a crash, and wherever they are it's rather nice that they get cleaned up automatically by the clean_temp program (if users wish to bypass normal startup and not run clean_temp, then it's their responsibility to look after that manually). What I am looking for with the dirname $GISRC idea is some way to replicate that auto-cleanup for GUI .ppm files that are forgotten, undeletable-at-runtime on Windows, and the residual results of the GUI crashing before cleaning itself up.

it's probably mainly of concern for large raster maps, where it's not a concern at all since in $MAPSET/.tmp/, but fwiw it's good to consider that /tmp/ is often on a much smaller filesystem than $GISDBASE. For example, v.in.ascii might make a multi GB tempfile during its scanning step.

Most uses of G_tempfile() are wrong

what's the potential downside of that? why "wrong" instead of "different"? i.e. is it theoretically wrong or practically wrong?

thanks, Hamish

comment:23 in reply to:  22 Changed 6 years ago by glynn

Replying to hamish:

Ok, if $GISRC is custom you'd get tmp files in weird places. (render.py could test if the getenv string started with "/tmp/grassX-" before using that, otherwise use $TMPDIR?)

The init script uses TMPDIR or TEMP or tempfile.gettempdir(), and only falls back to /tmp as the last resort.

Most uses of G_tempfile() are wrong

what's the potential downside of that? why "wrong" instead of "different"? i.e. is it theoretically wrong or practically wrong?

If /tmp (or $TMPDIR) is on a different filesystem to the mapset, it's likely to be faster, e.g. /tmp is likely to be on a local disc even if the mapset is on an NFS or CIFS share.

comment:24 Changed 6 years ago by hamish

ok so for the wxGUI temporary image files:

  • {$TMPDIR|%TEMP%}/ is where we are (spamming) now
  • $MAPSET/.tmp/ is out as it might be NFS'd and too laggy for GUI needs
  • dirname $GISRC is out as $GISRC might be custom set to anything
  • {$TMPDIR|%TEMP%}/grass$VER-$USER/ is out since can't safely clear during concurrent sessions

how about {$TMPDIR|%TEMP%}/grass$VER-$USER-$PID.gui/, falling back to {$TMPDIR|%TEMP%}/ if that wasn't set during init.sh/init.bat? (I'd rather not have the additional enviro variable just for it)

or check what dirname $GISRC looks like, and if it begins with $TEMPDIR/grass$VER-$USER-$PID/, use that, if not fall back to $TEMPDIR?

Hamish

comment:25 in reply to:  24 Changed 6 years ago by glynn

Replying to hamish:

how about {$TMPDIR|%TEMP%}/grass$VER-$USER-$PID.gui/, falling back to {$TMPDIR|%TEMP%}/ if that wasn't set during init.sh/init.bat? (I'd rather not have the additional enviro variable just for it)

Sure, but note that "$PID" needs to be $GIS_LOCK so that there's one temporary directory for the session, not one for each command.

comment:26 Changed 6 years ago by hamish

Milestone: 6.4.36.4.4

Some cleanup already in devbr6 with r56444, more proposed for trunk, but since it's not causing immediate breakage and this is a critical part of the GUI code, bumping it down the road rather than risk invoking the law of unintended consequences.

Hamish

comment:27 in reply to:  26 ; Changed 6 years ago by neteler

Replying to hamish:

Some cleanup already in devbr6 with r56444, more proposed for trunk, but since it's not causing immediate breakage and this is a critical part of the GUI code, bumping it down the road rather than risk invoking the law of unintended consequences.

Should this go into 6.4.svn or 7.0.x?

comment:28 in reply to:  27 Changed 6 years ago by neteler

Replying to neteler:

Replying to hamish:

Some cleanup already in devbr6 with r56444, more proposed for trunk, but since it's not causing immediate breakage and this is a critical part of the GUI code, bumping it down the road rather than risk invoking the law of unintended consequences.

Should this go into 6.4.svn or 7.0.x?

Should r56444 reach at least trunk (in modified form)?

comment:29 Changed 6 years ago by hamish

Keywords: v.digit ppm temp, wingrassv.digit, ppm, temp, wingrass

Should this go into 6.4.svn or 7.0.x?

yes, it should go into 6.4.svn at some point.

I'm not sure what I meant by "more [cleanup] proposed for trunk", but of course some solution is needed there too if it sees the same bug. someone running windows should be able to check if their C:\Documents and Settings\$USER\Local Settings\Temp (or similar) is full of old grass .ppm files.

-- looking at r56444 I'm reminded about this: "FIXME: the second RunCommand?() here locks up the GUI, need to use 'xkill' to recover."

which makes cairo unusable from the wxGUI on Grass 6.x (!). (#943) but that's another matter (help required by process-signals-on-Windows expert).

Hamish

comment:30 Changed 6 years ago by neteler

Manual (variables.html) : explained TMPDIR/TEMP/TMP env var usage for wxGUI settings in r59977 (and subsequently for all branches)

comment:31 Changed 5 years ago by annakrat

In r60946 (in 6.5) I tried to fix the ppm files not being removed from tmp folder. Testing is welcome of course, I tested it on Windows 8 and Ubuntu. I would suggest to backport it to have it in RC2 if you agree.

comment:32 Changed 5 years ago by annakrat

I committed it in r90947 to trunk.

comment:33 in reply to:  32 Changed 5 years ago by annakrat

Replying to annakrat:

I committed it in r90947 to trunk.

Backported to grass64 (r61089) and grass70 (r61090) so that somebody can test it.

comment:34 Changed 4 years ago by neteler

Is this solved?

comment:35 Changed 4 years ago by neteler

Resolution: fixed
Status: newclosed

No complaints, closing as solved.

Note: See TracTickets for help on using tickets.