Close the file after reading

[allwi290]

For further information see the following

Most operating systems limit the number of file descriptors that may be open at any given time so it is critical to close the descriptor when operations are completed. Failure to do so will result in a memory leak that will eventually cause an application to crash.

from https://nodejs.org/api/fs.html#fs_file_descriptors

[allwi290]

Hmmm, the main problem still exist but this is not the correct solution...

[constantinius]

Hi @allwi290 thanks for raising this issue and preparing a PR. As you correctly state, this is an actual issue, but as you also have experienced, the solution does not work. The fetch function is called multiple times, so it is not possible to close after the first call. I think it is very inefficient to open/close the file at every read, so we need a solution at a higher level.
I was thinking about a close method on both sources and the TIFF class, but this would need to be done by implementor, as this cannot be done automatically.
I'll think of something.

[allwi290]

I'm parsing rain radar images and after roughly 65000 images I'v got too many files open - error from the OS.... I my fork I have implemented that the file is opened and closed every time the fetch-method is called. I can post some benchmark result tomorrow.
Otherwise I think a close-method is perfectly fine! 😄

[allwi290]

Terrible slow if you open and close the file descriptor for each fetch...

[allwi290]

Terrible slow if you open and close the file descriptor for each fetch...

It was terrible slow due to my own misstake...

One shot benchmark
open geotiffs(450x800px) from file, getting the image, extracting 3x3 raster and repeating it for 65000 different files

The performance for open and closing the file for each fetch is the same as for keeping it open all the time. I think this will depend on the type of harddrive of the system SSD vs HDD

I have update my fork with this solution if you would like to try it out.

[constantinius]

@allwi290 I was thinking that you can actually solve this from the outside.

Lets say this is how you access the file:

const tiff = await fromFile(path);

Internally, this is nothing else than:

const tiff = await GeoTIFF.fromSource(makeFileSource(path));

The makeFileSource function is defined as:

export function makeFileSource(path) {
  const fileOpen = openAsync(path, 'r');
  return {
    async fetch(offset, length) {
      const fd = await fileOpen;
      const { buffer } = await readAsync(fd, Buffer.alloc(length), 0, length, offset);
      return buffer.buffer;
    },
  };
}

The idea is to create an own factory function that returns an object with both async fetch and async close:

function makeClosableFileSource(path) {
  const fileOpen = openAsync(path, 'r');
  return {
    async fetch(offset, length) {
      const fd = await fileOpen;
      const { buffer } = await readAsync(fd, Buffer.alloc(length), 0, length, offset);
      return buffer.buffer;
    },
    async close() {
       const fd = await fileOpen;
       closeAsync(fd);
    },
  };
}

Now you can handle the TIFF like:

const source = makeClosableFileSource(path);
const tiff = await GeoTIFF.fromSource(source);

// do something with the TIFF and then close the source
// ...

await source.close();

Please have a look at how readAsync is implemented so that you can have an idea how to implement closeAsync: https://geotiffjs.github.io/geotiff.js/source.js.html

[allwi290]

Hi
I tried that solution as well and it was working great.

The only drawback, I can see with this solution, is that you need to handle the file source differently from the other sources (you need to remember to close it). Am I correct?

Why do you want to create a new factory function instead of changing the existing one?
makeFileSource is only used by GeoTiff.fromFile

[constantinius]

That is true, sources of that type would need to be handled differently. On the other hand, a "close" on other sources would not make sense. I would also refrain from adding a close method on GeoTIFFs directly, as in most cases that would be a no-op.

I think I will integrate the above implementation in the built in factory function. On the other hand, if you have already implemented it, would you like to contribute your solution? We could simply reopen this PR for that purpose.

[allwi290]

@constantinius I can do it next week, if you can wait.

[allwi290]

@constantinius now you can review it

[constantinius]

Clean code, docs and tests, Very nice!
Thanks for fixing this issue