WinGRASS not deleting temp ppm files from map display

[isaacullah]

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.

[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

[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).

[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

[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

[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.

Hamish

[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

[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.

[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.

[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).

[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.

[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?

[neteler]

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?

[neteler]

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

[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

[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..)

[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

[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

[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).

[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

[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.

[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

[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.

[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.

Hamish

[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?

[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)?

[hamish]

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

[neteler]

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

[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.

[annakrat]

I committed it in r90947 to trunk.

[annakrat]

Replying to annakrat:

I committed it in r90947 to trunk.

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

[neteler]

Is this solved?

[neteler]

No complaints, closing as solved.