Opened 3 years ago

Closed 3 years ago

#23097 closed defect (fixed)

Maxima leaves a ton of compiled lips DLLs from ECL in /tmp when running tests

Reported by: embray Owned by:
Priority: critical Milestone: sage-8.0
Component: porting: Cygwin Keywords: windows cygwin ecl maxima
Cc: Merged in:
Authors: Erik Bray Reviewers: Emmanuel Charpentier
Report Upstream: Reported upstream. Developers deny it's a bug. Work issues:
Branch: 60b5d0f (Commits) Commit: 60b5d0f23b5cbb19780df784dcc8eee8a242e5bf
Dependencies: Stopgaps:

Description (last modified by embray)

I noticed a while ago that my /tmp directory was filling up often, and it seems to be that every time I run the Sage tests I'm left with ~650 temp DLLs generated by ECL totaling around 8 GB. This is even with a fully successful test run.

Just starting Maxima with sage -maxima causes three temp DLLs to be created, but they are properly deleted upon exiting. Likewise, running commands in Sage with the maxima interface creates DLLs, but they are cleaned up when Sage exits. It seems like maybe when running the test suite Maxima isn't always exited cleanly, and the temp files do not get tidied up.

Upstream report: https://gitlab.com/embeddable-common-lisp/ecl/issues/388

Change History (15)

comment:1 Changed 3 years ago by embray

  • Keywords maxima added

comment:2 Changed 3 years ago by embray

  • Description modified (diff)
  • Summary changed from Tests leave a ton of compiled lips DLLs from ECL in /tmp to Maxima leaves a ton of compiled lips DLLs from ECL in /tmp

comment:3 Changed 3 years ago by embray

  • Description modified (diff)
  • Summary changed from Maxima leaves a ton of compiled lips DLLs from ECL in /tmp to Maxima leaves a ton of compiled lips DLLs from ECL in /tmp when running tests

comment:4 Changed 3 years ago by embray

I see--these temp files get created by ECL on Windows/Cygwin when loading a compiled module and force_reload feature is enabled. In order for this to work on Windows a copy of the module needs to be made and loaded, since Windows does not allow loading the same DLL more than once in the same process. ECL takes care to clean this up during proper shutdown with an atexit() handler, but if the test suite is just killing processes that may prevent the handler from being run.

Last edited 3 years ago by embray (previous) (diff)

comment:5 Changed 3 years ago by embray

I'm trying to decide what would be best to do about this. There are a few possibilities, none of which are ideal:

a) Patch ECL so that it handles more exit signals (SIGTERM, SIGHUP, etc.) to shut down cleanly / delete temp files. This won't help for SIGKILL, but in most cases (during the tests) I think the maxima processes are just being SIGTERM'd (but this needs to be confirmed). So that would fix most issues. Actually, this should probably be done regardless--there's no reason ECL couldn't shutdown more cleanly on signals.

b) Do the cleanup from Sage. This has the advantage that it can even handle an unclean shutdown of ECL. The only problem is I don't see an obvious way to get a list of temp files created by a given ECL process. If there is only one ECL, no problem. But it shouldn't clean up temp files used by another ECL. Granted, for the limited case on Windows, which is the only place this applies, trying to delete temp DLLs still in use by a process should fail, so it might work just to clean all ecl*.dll from /tmp...

comment:6 Changed 3 years ago by embray

  • Description modified (diff)
  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

Reported upstream to ECL since I feel this should be fixed there. Haven't attempted a fix myself yet.

comment:7 Changed 3 years ago by embray

  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:8 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/cygwin/ticket-23097
  • Commit set to 60b5d0f23b5cbb19780df784dcc8eee8a242e5bf

I haven't attempted to patch ECL for this yet; its signal handling code is a bit complex and I think I need to understand it better before doing so.

However, this provides a workaround that I think is worth having with or without a patch to ECL for this: This ensures that when ECL and/or Maxima are called from Sage, rather than writing temp files to /tmp it uses SAGE_TMP. This way an individual Sage session can take responsibility for cleaning up any temp files created by ECL/Maxima processes it spawns (or that it creates through the sage.libs.ecl interface).

As far as I'm concerned this solves the problem from Sage's end.


New commits:

e19e609Add ability to pass environment variable overrides to the Expect interface
dd695e5Add a new ECL_TMP lazy_string that returns a path to the temporary directory ECL should use when running from Sage
60b5d0fUpdate the sage.libs.ecl interface to set ECL's temp dir to ECL_TMP

comment:9 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:10 Changed 3 years ago by charpent

  • Reviewers set to Emmanuel Charpentier
  • Status changed from needs_review to positive_review

Reviewers set to Emmanuel Charpentier (revert) Status changed from needs_review to positive_review

On a Virtulbox running Windows10 professional, I compiled 8.0.rc0 + #21399 + #23359 (as suggested in ​the Wiki) + #21233 + #23097 (present ticket) with the options :

export PREREQ_OPTIONS=--with-blas=atlas export SAGE_ATLAS_LIB=/usr/lib export MAKE="make -j4"

This passes ptestlong with no failures.

==> positive_review

Note : I have no advice on the contents of the patches (I do not know Cygwin well enough to undetstand what they are supposed to do) ; I just checked that this leads to a functional Sage.

comment:11 Changed 3 years ago by embray

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Reported upstream. Developers deny it's a bug.

For what it's worth, the ECL developers decided this is a "wontfix" after all. I'm not entirely sure I agree but not gonna fight them on it.

comment:12 Changed 3 years ago by chapoton

milestone ?

comment:13 Changed 3 years ago by embray

  • Milestone set to sage-8.0

comment:14 Changed 3 years ago by embray

  • Priority changed from major to critical

Not sure why the milestone was missing from this. This is actually pretty critical to have :/

comment:15 Changed 3 years ago by vbraun

  • Branch changed from u/embray/cygwin/ticket-23097 to 60b5d0f23b5cbb19780df784dcc8eee8a242e5bf
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.