tests failing in core on Windows

teleivo · July 17, 2020, 12:13pm

its pretty clear to me now from the error message

Suppressed: java.nio.file.FileSystemException: C:\Users\Cliff\AppData\Local\Temp\junit1581269591057766963\TestingComplexObsSaving_9064e534-7f4b-45be-b266-a78bd131b7fb.mp3: The process cannot access the file because it is being used by another process.

that something is holding on to the file/stream in these 2 tests that are failing. So either its our test that needs to close them or the implementation has a bug that hasn’t been detected. I will investigate further. I will ask @gcliff to try again as soon as I have changed things. Thank you for letting us know

gcliff · July 17, 2020, 12:17pm

thanks @teleivo for your investigations on this

teleivo · July 17, 2020, 12:40pm

@gcliff please pull and try again. I adapted the tests to close the input streams it creates. I hope this fixes the tests on Windows let us know

gcliff · July 17, 2020, 1:04pm

@teleivo its still misbehaving https://pastebin.com/gqJQ61Nv

teleivo · July 17, 2020, 1:09pm

Out of curiosity whats in the temp dir that cannot be deleted by JUnit? The one it shows when the test fails C:\Users\Cliff\AppData\Local\Temp\junit3772331837415344270 And is there any difference when you run this from your IDE?

teleivo · July 17, 2020, 1:13pm

and @gcliff please replace

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/obs/handler/BinaryStreamHandler.java#L117


	 * @see ComplexObsHandler#saveObs(Obs)
	 */
	@Override
	public Obs saveObs(Obs obs) throws APIException {
		try {
			// Write the File to the File System
			String fileName = obs.getComplexData().getTitle();
			InputStream in = (InputStream) obs.getComplexData().getData();
			File outfile = getOutputFileToWrite(obs);
			OutputStream out = new FileOutputStream(outfile, false);
			OpenmrsUtil.copyFile(in, out);
			
			// Store the filename in the Obs
			obs.setComplexData(null);
			obs.setValueComplex(fileName + "|" + outfile.getName());
			
			// close the stream
			out.close();
		}
		catch (Exception e) {
			throw new APIException("Obs.error.writing.binary.data.complex", null, e);

with FileCopyUtils.copy(in, out); with import import org.springframework.util.FileCopyUtils; The FileCopyUtils from Spring closes the in and outputstream.

I would be interested if that is solving it. Then we should have closed all stream/handles on the files JUnit wants to delete. At least that is my hope

teleivo · July 17, 2020, 1:46pm

Am interested to know if my suggestion changed anything for you @gcliff

What I am struggling to understand is

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/obs/handler/MediaHandler.java#L105


	 * @see org.openmrs.obs.ComplexObsHandler#saveObs(org.openmrs.Obs)
	 */
	@Override
	public Obs saveObs(Obs obs) throws APIException {
		
		try {
			// Write the File to the File System
			String fileName = obs.getComplexData().getTitle();
			File outfile = getOutputFileToWrite(obs);
			OutputStream out = new FileOutputStream(outfile, false);
			FileInputStream mediaStream = (FileInputStream) obs.getComplexData().getData();
			OpenmrsUtil.copyFile(mediaStream, out);
			
			// Store the filename in the Obs
			obs.setComplexData(null);
			obs.setValueComplex(fileName + "|" + outfile.getName());
			
			// close the stream
			out.close();
		}
		catch (IOException ioe) {

and

github.com

openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/obs/handler/BinaryStreamHandler.java#L114


	}
	
	/**
	 * @see ComplexObsHandler#saveObs(Obs)
	 */
	@Override
	public Obs saveObs(Obs obs) throws APIException {
		try {
			// Write the File to the File System
			String fileName = obs.getComplexData().getTitle();
			InputStream in = (InputStream) obs.getComplexData().getData();
			File outfile = getOutputFileToWrite(obs);
			OutputStream out = new FileOutputStream(outfile, false);
			OpenmrsUtil.copyFile(in, out);
			
			// Store the filename in the Obs
			obs.setComplexData(null);
			obs.setValueComplex(fileName + "|" + outfile.getName());
			
			// close the stream
			out.close();

since the OpenmrsUtil.copyFile(in, out); does not close the InputStream I believe the saveObs should then close them.

@dkayiwa what is your take on this?

gcliff · July 17, 2020, 1:48pm

when i run from the IDE is till get this error stacktrace

i have tried using FileCopyUtils.copy(in, out) from spring and on running the test in the IDE i get java.io.IOException: Failed to delete temp directory C:\Users\Cliff\AppData\Loca - Pastebin.com

teleivo · July 17, 2020, 1:49pm

both tests are still failing?

gcliff · July 17, 2020, 1:50pm

its a appdir for unit tests

teleivo · July 17, 2020, 1:50pm

if anyone has a good idea how we can figure out what is holding a reference to these temporary files that are created during the tests. please let me know

gcliff · July 17, 2020, 1:53pm

yes sir …

gcliff · July 17, 2020, 1:57pm

@teleivo how about i delete the folders inside the Temp folder and then try to build again …

May be if i may ask ,are these numerous appdir-for-unit-tests files in the Temp folder generated automatically during maven build ?

@ibacher @dkayiwa

teleivo · July 17, 2020, 5:14pm

@gcliff I meant what is inside the folders. I am assuming the mp3 files created during the test.

I will disable the tests on Windows using @achilep proposal for now. The tests will still run in all our pipelines which ensures the behavior of the code will not change. In the meantime I will try to replicate your environment and the errors you are seeing. So that I can investigate more easily and look for a fix

teleivo · July 17, 2020, 5:39pm

@gcliff I pushed @achilep suggestion. So these 2 tests that fail on your machine should now be disabled on Windows.

teleivo · July 17, 2020, 5:59pm

This will be hard So @gcliff my research tells me you run Windows 7, correct? I was able to run the tests inside a Microsoft provided docker image with JDK8 https://hub.docker.com/_/microsoft-java-jdk but I assume that was naive since that is most likely very different from actual Windows since its meant for deploying things to Azure.

for the curious this is what you do

docker pull mcr.microsoft.com/java/maven:8-zulu-debian9
docker run -it --name windows -v ~/.m2:/root/.m2 -v ~/code/openmrs/openmrs-core:/code --workdir /code mcr.microsoft.com/java/maven:8-zulu-debian9 sh
mvn clean test -Dtest=BinaryStreamHandlerTest,MediaHandlerTest

I will most likely need to setup a VM. Not sure yet if I can get a Windows 7.

ibacher · July 17, 2020, 6:09pm

Just to point this out, you’ve actually pulled a Microsoft-provided Debian image, which really doesn’t help with the Windows-specific file handling issue. The minimal Windows image seems to be called 8-zulu-windowsservercore…

ibacher · July 17, 2020, 6:23pm

They’re generated here in the BaseContextSensitiveTest class which means that we generate probably 200ish directories per Maven build. They should be deleted on exit, but apparently some aren’t. Although I don’t get the exception you’re reporting on my machine (I use a Mac), I do see a number of those appdir-for-unit-tests- directories piled up in my temporary files location. When I clean the temp directory and re-run the tests, I end up with 11 hanging appdir-for-unit-tests- directories.

ibacher · July 17, 2020, 6:28pm

Here is the full listing of files that aren’t getting cleaned up on exit (for my machine).’

@gcliff If you could get a similar listing for your machine, it might be helpful in tracking down where we have unclosed files.

teleivo · July 17, 2020, 6:39pm

Thanks at @ibacher, if you remove the @DisabledOnOs(WINDOWS) flag on for example the MediaHandlerTest does it fail telling you the Jupiter Extension could not delete the temp dir because some other process was holding on to things in there (like in @gcliff logs)?

I just ran spotbugs on openmrs-core. Both tests that fail on Windows have a Bug descriptions — spotbugs 4.7.1 documentation on the getObs() method. They are both creating an InputStream pass them on to the ComplexData.data and never close it. See here for example openmrs-core/MediaHandler.java at master · openmrs/openmrs-core · GitHub

I just pushed TRUNK-5816 Attempt fix failing HandlerTests on Windows · openmrs/openmrs-core@6dc1a74 · GitHub in an attempt to close them myself in the test.

Can you @ibacher and @gcliff please run the MediaHandlerTest and BinaryStreamHandlerTest (remove the @DisabledOn) and let me know if that changes anything?