Test 48490: Temporary directory cannot be deleted after an import

[lukas-heinrich]

Hello everyone,

This PR addresses the issue described in Ticket 0048490. There is a problem while deleting the temporary import directory after a test has been imported successfully. Even though the import is completed and the test object was created, the user is facing an error screen.

We were able to trace the issue to a race condition that occurs when importing large test objects, caused by the file handlers not being closed correctly after the upload is extracted. As a result, the directory cannot be completely deleted during cleanup. According to our analysis, NFS file systems are particularly affected by this issue.

In my opinion, the best way to resolve this issue is to release this handler in the Unzip destructor. For this reason, I've introduced the object destructor to the Unzip class. The second change in the test component is an additional safeguard that deletes that object instance as early as possible.

I'd appreciate any feedback or suggestions for alternative solutions.
Best
@lukas-heinrich

/cc @thojou

[chfsx]

This does what PHP already does when the ZipArchive is freed, so it does not change the lifetime of the handle. Additionally, since the return value of open() is not checked in the constructor, error_reading_zip can be false for an archive that was never opened, and close() would then throw a ValueError from the destructor. I'd rather release the resources at the end of extract().

[chfsx]

This is the change that actually fixes the issue here, but only for this consumer. If Unzip::extract() releases its handles itself, this is no longer necessary.

[chfsx]

Hi @lukas-heinrich, thanks for tracking this down, the analysis regarding open handles on NFS is right. However, I think this PR only fixes the symptom in one consumer and not the cause in Unzip.

• The destructor is effectively a no-op: ZipArchive is closed by PHP anyway when the object is freed, and the fopen handle held in $this->stream is closed by Stream::__destruct(). The only change that actually makes a difference is the unset($unzip) in ilObjTestGUI, because importVerifiedFileObject(true) (and with it delDir() / deleteUploadedImportFile()) runs while $unzip is still in scope.
• The same pattern (unzip(Streams::ofResource(fopen(...))), then cleanup in the same scope) exists in many other consumers. No consumer should have to unset() the Unzip object to be able to delete the ZIP afterwards.
• The destructor introduces a new risk: ZipArchive::open() does not throw on failure but returns an error code, which is not checked in the constructor, so error_reading_zip stays false for broken files. close() on a not opened archive throws a ValueError in PHP 8, which would then be thrown from a destructor.

I'd suggest fixing this in Unzip itself instead:

• extract() is a terminal operation, so it should release its resources ($this->zip->close() and $this->stream->close()) in a finally block.
• Check the return value of ZipArchive::open() (!== true -> error_reading_zip = true).
• Add a unit test: extract a temporary ZIP and delete the ZIP and its directory in the same scope.

With that, the unset() in ilObjTestGUI is no longer needed.

I'll take care of the implementation myself: I'd like to fix the cause in Unzip rather than working around the symptom in single consumers, and I want the fixed behaviour to be covered by unit tests in the Filesystem component. I'll link the PR here once it is ready.

[chfsx]

As announced, the fix in Unzip is available in #12204. With it, the ZIP-file is no longer kept open after extract(), so the unset() in ilObjTestGUI is not needed anymore.