InputFactory does not release RandomAccessFile

[vforchi]

It looks like the method InputFactory.createFileFactory(File, long, long) does not release the resources allocated by the RandomAccessFile, therefore some file descriptors are not released.

        if ( leng <= BlockMappedInput.DEFAULT_BLOCKSIZE * 2 ) {
            // cut
            RandomAccessFile raf = new RandomAccessFile( file, "r" );
            final FileChannel chan = raf.getChannel();
            return new AbstractInputFactory( true ) {
                public BasicInput createInput( boolean isSeq )
                        throws IOException {
                    return new SimpleMappedInput( chan, offset, ileng,
                                                  logName );
                }
            };
        }

[mbtaylor]

Yes, that's probably true. As long as a StarTable is around, it's assumed that one may want to perform data access on it. In the case of a table based on a FITS BINTABLE extension backed by an uncompressed file on disk, it seems reasonable to keep the file descriptor of the relevant file; in any case that makes the data access likely to be more simple/robust/efficient. So, I don't think I regard it as a bug/misfeature, though admittedly there's no way to force it to release the file descriptor other than waiting for it to be garbage collected (when I assume the FileChannel or RandomAccessFile will relinquish resources, though I haven't tested that).

If you're trying to do something specific that's running into this issue, there are probably ways to work around the problem - I'm happy to advise if you want to give me more details.

[vforchi]

I'm just creating a lot of tables one after the other, and the gc doesn't kick in. I tried to force it, but with no luck, I think the fds are not released in this case.

I understand the use case you describe, but wouldn't it be better to add a close method to the StarTable to release resources?

At the moment I have to process ~10k files, read the data and perform some basic checks.

[mbtaylor]

Adding to the StarTable interface a new close method, which implicitly should be called when the object is no longer in use, changes the contract of that interface. There is a lot of application code using StarTables which then ought to be changed, and in many cases it would be hard to keep track of the usage well enough to know when the close should be called. And in many (I think most) cases, actual StarTable implementations are not hanging on to non-obvious resources. So, I'm not keen to solve this by a change to the StarTable API itself.

But, I understand your predicament. I've got two suggestions.

The first is to open the files in such a way that they avoid this code. The most respectable way to do it would be to use the TableBuilder.streamStarTable method, which streams the data in one pass to a data consumer (TableSink) rather than creating a StarTable object that's supposed to be able to read the data multiple times as required. This won't retain any reference to the underlying file; or at least if it does I'll admit it's a bug :-). That may or may not be suitable or convenient for what you're doing.

Alternatively, just specify the input file in such a way that it avoids the code you cite above. I'm not sure how you're creating your StarTables, but basically if the code thinks the data is just coming from a stream and not a mappable file, it won't keep a reference to the file. So using e.g. a URLDataSource rather than a FileDataSource should avoid the problem, e.g. doing

   StarTableFactory.makeStarTable("file://localhost/path/t.fits") 

rather than

   StarTablefactory.makeStarTable("/path/t.fits")

But if you're unhappy with any of those options, I could consider adding a close method or similar to StarTable concrete subclasses that actually need it (BintableStarTable). Then you could either get your StarTables by constructing BintableStarTables explicitly or just cast them to BintableStarTable where appropriate, and call close() where required.

[vforchi]

Although hanging the interface would require the users to do something more, I don't like the idea of resources not being freed, and not having a way to free them.

On the other hand, I will try to change the datasource and see if that, together with the other fox you made yesterday, solves my problem.

What I don't like is that I don't know what classes keep fds open and what won't. Generally speaking, as a user I don't want to know, since it's an implementation detail.

[vforchi]

I replaced all the FileDataSource usages with URLDataSource, except for one, where I use the constructor FileDataSource(String, int): is there a workaround in this case?

[mbtaylor]

Yes, when using a URL the fragment identifier is used for the position, so you can replace new FileDataSource("/path/file.fits",3) with new URLDataSource("file:///path/file.fits#3").

I do admit that having to do this kind of workaround is evidence of bad design, and that users shouldn't need to know which classes hang on to fds or other resources. In fact even I don't know which classes hang on to fds. But the impact of adding a close method to the StarTable interface, which is right at the core of STIL/STILTS/TOPCAT means I'm reluctant to do it. Your use case of opening thousands of FITS files is certainly legitimate, but it's obviously not common, since I don't think this has come up before now. And there are cases even in the J2SE of similar limitations (java.nio.MappedByteBuffer) so at least I'm in good company :-).

Here's another possibility. I could add a switch somewhere (public static boolean member of BintableStarTable?) that is off by default, but if on prevents implementations from hanging on to fds, most likely at some cost to performance. This is a hack, but it would mean that your code doesn't need pervasive and opaque workarounds. What do you think?

[vforchi]

I have to say, I don't like the switch either.

Why don't you add the close() method to the FitsTableBuilder? It should be relatively easy to keep the references there. In principle you could add it to the TableBuilder, but then we are back at the same API problem you mentioned with the StarTable.

[mbtaylor]

FitsTableBuilder is an object factory, and it doesn't keep any record of the objects it has created, so I'm not going to keep the references there. I could add a method something like this:

    public class FitsTableBuilder implements TableBuilder {
        ...
        public void close(StarTable table) {
            if (table instanceof BintableStarTable) {
                ((BintableStarTable) table).close();
            }
        }

But note this could still end up with problems that are not my fault - as noted in the FitsTableBuilder javadocs, as well as an fd, this implementation also comes with a MappedByteBuffer, and as I noted above that sits on non-reclaimable resources as well (MappedByteBuffer javadocs say "A mapped byte buffer and the file mapping that it represents remain valid until the buffer itself is garbage-collected"). Now I do have some unrespectable hacks around that particular problem elsewhere in the code where it's proved unavoidable, but I'm not sure I want to bring them in here.

Another option is to add a parameter to the FitsTableBuilder constructor that flags whether it tries to give you these efficient-but-non-resource-releasing implementations as it does now, or restricts itself to less problematic (and maybe less efficient) implementations. That's probably a better way to do it than a public static member, as long as you have control over the FitsTableBuilder instances you're using.

[vforchi]

In that case I might as well call the close() method on the table myself, but you have to bring the reference to the fd up to the table, which is not the case right now.

I'm actually ok with that implementation, although I would prefer it to have it in the interface, since it would be backwards compatible.

[mbtaylor]

So do you want a new close method on BintableStarTable, or are you asking for something different? As I say, that does give you a way to work round fd exhaustion, but might still leave you with mapped memory problems.

[vforchi]

Yes, I think that's the simplest solution to my problem, since I'm only facing it with the BintableStarTable.

[mbtaylor]

I've made a fix at 5eac8e46a8 so that BintableStarTable (and, FWIW, ColFitsStarTable) now implement the java.io.Closeable interface, so you can write (slightly more respectably than my earlier suggestion):

     if (t instanceof Closeable) ((Closeable) t).close();

and I hope it should release all the fds. Behaviour is undefined if you try to access the data after the close call. Note the above idiom is not going to work in general as a way to release resources in STIL application code since many StarTable instances that haven't just been created are wrapper instances that won't inherit the wrapped table's interfaces.

Let me know if this solves your problem.

[vforchi]

Thanks for the quick fix. Now it's much better: my test passes (it runs some validation routine 10k times), although I monitored the number of open fds, and it goes up to 200, and I wouldn't expect it to be more than a few fds. But at the moment I cannot tell whether it's my code or yours.

By the way, I noticed you are including an old version of nom.tam.fits, which has a number of problems: is there a specific reason why you didn't upgrade? The library recently moved to github and is also available from maven (Maybe this is topic for another ticket).

[mbtaylor]

I know I'm using a very old nom.tam.fits. I have made some very small bugfix-type alterations to it over the years, and it works for my purposes, so I don't want to upgrade it and risk different behaviour that might cause trouble.

The parts of nom.tam.fits that I use are pretty limited. It does the ASCII FITS table handling, but I think that's rarely used (there aren't many ASCII FITS tables out there). For BINTABLEs I do use the header parsing from nom.tam.fits, as well as some of the interfaces, but most of STIL's FITS BINTABLE implementation is custom code, with performance characteristics that are (at least were when I wrote it) much better for my purposes than nom.tam.fits. I doubt if upgrading to recent versions of nom.tam.fits would bring benefits for me.