Leak: openInput, openOutpt, SourceCloser, DestCloser pattern is fraught

[m-7761]

Lately I've been looking at the model file "filters" just because I removed release_ptr that they had used.

They suffer from a lot of the same design patterns as the texture filter system. A big problem throughout the code is it's written oddly defensively, to the point of parody, as if it doesn't know what its purpose is. So you get weird patterns like implementations of interfaces are checking inputs for null pointers, and reproducing error codes, instead of streamlining these interfaces to be plugged into their so-called managers.

Like with textures, the DataDest and DataSource objects should be fed to these filters... obviously. They shouldn't be reimplementing file opening code, or even be limited to the local file system. Similarly the model writers (exporters) are weirdly creating their own "default options" objects. So what I'm saying is, I'm restructuring these.

Another weird pattern is the model filters are using openInput and openOutput, which shouldn't even exist. These are memory leaks, since these new allocate objects that don't have corresponding delete procedures. There objects should be created on the stack, but they should be passed to filters in place of path strings too. There is a SourceCloser and DestCloser pseudo RAII like pattern that is also deployed on them, but these just close the file handles.

Part of the reason there are so many leaks in the code, is a lot of the patterns are unsound. But I'm not being overly critical, since sound code is not as impressive as functional code. But ideally, you don't settle for less than both of these things.

Anyway, the simple fix here is to rip out these unnecessary constructs/routines and just use constructors. But I expect I will get around to upgrading the "manager" to at least pass DataDest and DataSource directly to the filters... I changed it this morning to pass Options by reference.

[m-7761]

Well, here's more. I confused getDest/createDest etc.

The getDest implementation below has a map of files... but it always calls createDest so it's still leaking at that point, since it doesn't look for the existing file. I don't follow what the significance of this factory pattern is. If it's to keep files open, it can't... since they are being closed... then if it's to avoid memory allocations, it's doing no good. But neither is keeping files open any good either.

It's limited to the model filter system. In that case, I think it's wisest to rip it out. I see not rationale or value behind it. But it does seem (maybe) to have a garbage-collection function. The manager calls FileFactory::closeAll after it invokes the filters. But it's hard to say since there is a "default factory".

DataDest *FileFactory::getDest(const char *filename)
{
    DataDest *dst = createDest(filename);
    m_dests[filename] = dst;
    return dst;
}

DataSource *FileFactory::getSource(const char *filename)
{
    DataSource *dst = createSource(filename);
    m_sources[filename] = dst;
    return dst;
}

DataDest *FileFactory::createDest(const char *filename)
{
    return new FileDataDest(filename);
}

DataSource *FileFactory::createSource(const char *filename)
{
    return new FileDataSource(filename);
}

[m-7761]

EDITED: I think one possible use case for this kind of interface could be to represent a multi-file model in memory. -There's no comments to communicate their purpose.-

It's just an idea I had looking at what is consuming openInput/openOutput. Md3Filter seems to have animations stored in separate files.

There's a comment in ModelFilter:

    // Overrides the default FileFactory with a custom version.
    // This can be used to change how files are read and written by
    // any file filters that use the ModelFilter's openInput and
    // openOutput functions.
    void setFactory(FileFactory *factory){ m_factory = factory; }

And there is a comment at the top of the file too. I guess I expected to find comments above the class:

// The FileFactory creates DataSource and DataDest objects. It is used by
// the ModelFilter class so that file I/O can be replaced with memory I/O.
//
// To implement a different FileFactory type, override createDest and
// createSource to create the DataDest and DataSource types of your choice.

I suppose it's a bit more complicated. getDest/getSource are should be rewritten to not leak. But it's hard to say what is right beyond this.

I think FileSystem would be a better name to communicate the intent of the class. It would better to be able to pass the initial I/O object to the filter, but how to proceed with multi-file models is a different matter. I think OBJ may store materials separately.

EDITED: Some of the filters are even doing the following to their input paths:

    std::string modelPath = "";
    std::string modelBaseName = "";
    std::string modelFullName = "";
    normalizePath(filename,modelFullName,modelPath,modelBaseName);

I intend to remove this pattern (and use URLs ideally) that seems ungainly, but it seems like the sort of thing that the manager/caller could at least guarantee. My feeling is that user code should not interact with filters directly, since that puts more burden on the filter.

[m-7761]

I feel like this is a really bad waste of time... but anyway, below is code that demonstrates plugging this leak. Unfortunately the semantics of the FileFactory object are not at all clear...

I mean I'm pretty sure "getDest" should be equivalent to accessing the map. But if so, then it probably shouldn't open the file automatically. And it is unclear what to do if an existing file is opened or closed. Of course, the class seems not designed to access the same file more than once. I hate this sort of thing. It's why simpler is probably better.

EDITED: It does have the following API that does not have the same semantic baggage as the other. I feel like openDest, openSource would be better names in that case.

        SourceMap &getSourceMap(){ return m_sources; }
        DestMap &getDestMap(){ return m_dests; }

(It's not necessary to use "insert" here, but it's usually good practice to not do double look-ups.)

(Edited: Using delete here is just doing what closeAll and ~FileFactory does.)

DataDest *FileFactory::getDest(const char *filename)
{
    //I don't know if this is an improvement, but at least it 
    //doesn't leak.
    //DataDest *dst = createDest(filename);
    //m_dests[filename] = dst;
    //return dst;
    DataDest* &dst = m_dests[filename];
    delete dst;
    return dst = createDest(filename);
}

DataSource *FileFactory::getSource(const char *filename)
{
    //I don't know if this is an improvement, but at least it 
    //doesn't leak.
    //DataSource *dst = createSource(filename);
    //m_sources[filename] = dst;
    //return dst;
    DataSource* &src = m_sources[filename];
    delete src;
    return src = createSource(filename);
}

[m-7761]

Hate to harp, but here is a deep dive into the factory business...

The reason this perplexed me so, is there is a default (unspecialized) factory built into the ModelFilter object. It's not a global factory. However, there is another m_factory in FilterManager that is not a pointer. And when the filter is added to the manger, it sets (setFactory) the filters to use its (vanilla) factory.

It seems self-defeating. And it also means that you can't use a custom factory with the manager class if you wanted to. (Well you could set it back to your factory after adding it. I think probably this behavior should changed or removed, but it's small potatoes.)

I don't see a reason to have a factory in the manager myself. Maybe a pointer that is null by default, as a convenience, but not like that.

FWIW I've been simplifying the filters and managers around their formats to just return a C-string of all-caps space separated file extensions, and have make the other methods use those strings, so they are not virtual methods. It's much simpler.

They were building lists of std::string on the fly to communicate their formats, and reimplementing the same file-extension parsing logic everywhere, instead of in one place. The amount of code to convert that into filter strings for the open/save dialogs was over the top. But as it stands, it will be necessary to convert the plain extensions into the "*.ext" style strings somehow in the UI interfacing code.

[m-7761]

EDITED: I've changed the leak plugging code to not use insert. It was incorrect in its test, but I realized the test was not needed, since the default is nullptr, and a reference could be used to not require insert. I really wish I never made this topic, but I think the subject (problem) is kind of interesting to think about. I just don't know if it will ultimately be relevant or not.