Ticket #560 (new defect)

Opened 4 years ago

Last modified 6 days ago

WinGRASS not deleting temp ppm files from map display

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

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

follow-up: ↓ 2   Changed 4 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

in reply to: ↑ 1   Changed 4 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).

  Changed 3 years ago by hamish

  • keywords temp, wingrass added; temp removed

  Changed 3 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

  Changed 2 years ago by hellik

  • milestone changed from 6.4.0 to 6.4.2

  Changed 16 months 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

  Changed 16 months ago by martinl

  • cc martinl added

follow-up: ↓ 9   Changed 16 months ago by hamish

  • priority changed from major to critical

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

in reply to: ↑ 8 ; follow-up: ↓ 10   Changed 9 months 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

in reply to: ↑ 9 ; follow-up: ↓ 11   Changed 9 months 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.

in reply to: ↑ 10 ; follow-up: ↓ 12   Changed 9 months 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.

in reply to: ↑ 11 ; follow-up: ↓ 13   Changed 9 months 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).

in reply to: ↑ 12 ; follow-up: ↓ 14   Changed 9 months 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.

in reply to: ↑ 13 ; follow-up: ↓ 15   Changed 2 months 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?

in reply to: ↑ 14   Changed 6 weeks ago by neteler

  • milestone changed from 6.4.2 to 6.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?

  Changed 6 weeks ago by neteler

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

  Changed 3 weeks 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

  Changed 3 weeks 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..)

  Changed 6 days 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

Note: See TracTickets for help on using tickets.