2
\$\begingroup\$

This code is code written on a whim to help $someindividual asking for such a thing on $somechatmedium; what is needed is a way, in Java, to provide an Iterator over Path objects which must only be directories, recursively, and where the number of returned instance number in some sufficient quantities to trigger an OutOfMemoryError if eagerly populated.

So, I came up with this (imports omitted for "brevity"):

public final class LazyDirectoryIterator
    implements Iterator<Path>, Closeable
{
    private static final int QUEUE_SIZE = 5;
    private static final long TIMEOUT_QUANTITY = 100L;
    private static final TimeUnit TIMEOUT_UNIT = TimeUnit.MILLISECONDS;

    private final ExecutorService executor
        = Executors.newSingleThreadExecutor();
    private final BlockingQueue<Path> queue
        = new ArrayBlockingQueue<>(QUEUE_SIZE);

    private Path nextPath;
    private boolean needNext = true;

    public LazyDirectoryIterator(final Path baseDir)
    {
        executor.submit(new Runnable()
        {
            @Override
            public void run()
            {
                try {
                    Files.walkFileTree(baseDir, new CustomVisitor());
                } catch (IOException ignored) {
                    Thread.currentThread().interrupt();
                }
            }
        });
    }

    @Override
    public boolean hasNext()
    {
        try {
            if (needNext) {
                nextPath = queue.poll(TIMEOUT_QUANTITY, TIMEOUT_UNIT);
                needNext = false;
            }
            return nextPath != null;
        } catch (InterruptedException ignored) {
            Thread.currentThread().interrupt();
            return false;
        }
    }

    @Override
    public Path next()
    {
        if (!hasNext())
            throw new NoSuchElementException();
        needNext = true;
        return nextPath;
    }

    @Override
    public void remove()
    {
        throw new UnsupportedOperationException();
    }

    @Override
    public void close()
        throws IOException
    {
        executor.shutdownNow();
    }

    private final class CustomVisitor
        extends SimpleFileVisitor<Path>
    {
        @Override
        public FileVisitResult preVisitDirectory(final Path dir,
            final BasicFileAttributes attrs)
            throws IOException
        {
            try {
                queue.put(dir);
                return FileVisitResult.CONTINUE;
            } catch (InterruptedException e) {
                Thread.currentThread().interrupt();
                throw new IOException("timeout", e);
            }
        }
    }
}

There are three places here where I purposely choose to set the interrupt flag:

  • in the Runnable walking the file tree, if I encounter an I/O exception (I'm really not sure about this one; but a Runnable can't throw);
  • in the FileVisitor itself, if I can't push onto the queue;
  • in .hasNext() if trying and retrieving an element from the queue times out.

It is expected that this Iterator will never run in more than one thread, which is why needNext and nextPath are not volatile. Note that in the three usages above, the two first are in another thread which is spawned from the constructor.

Note: up to Java 7, cannot use Java 8.

So, is my handling of InterruptedExceptions, and my uses of Thread.currentThread().interrupt() correct?

\$\endgroup\$
2
\$\begingroup\$

Overall your code looks fine to me, however some remarks:

  1. I would log the IOExceptions that may happen during walkFileTree. Otherwise, if there are problems with the tree walk, it's hard to find out the reason later (e.g., if the baseDir does not exist). Perhaps you should perform also some sanity checks (i.e, baseDir is not null and baseDir exists) before submitting the Runnable.
  2. When catching the IOException, it would be also possible to just throw a RuntimeException that has the IOException as its cause. But this is just a matter of taste. However, in both cases if an IOException is thrown you need to be aware that the ExecutorService has still an idle thread (that may prevent the shutdown of your app) until you call the close method on LayzDirectoryIterator. Maybe using a custom ThreadFactory that creates daemon threads (and neat thread names) is an option here? I like to use Guava's ThreadFactoryBuilder for this purpose.
  3. I'm generally not a fan of spawning a new Thread within the constructor, I would add an additional start method to submit the tree walk Runnable.
  4. Your Thread.currentThread().interrupt() within the CustomVisitor is fine.
\$\endgroup\$
1
\$\begingroup\$

As you mentioned Java 7. It would be easier if you implemented AutoCloseable instead of Closeable (see also: this SO answer)

For some reason To preserve backward compatibility language designers decided to create a separate one. This way not only all Closeable classes (like streams throwing IOException) can be used in try-with-resources but also those that wish throwing more general checked exception from close().

When in doubt, use AutoCloseable, users of your class will be grateful.

Just like s106mo mentions in his answer I am also not a fan of spawning a new Thread in your constructor. Unfortunately exactly that is the responsibility of a constructor: getting your class into a working state, that's why I think this solution is the lesser "evil" and would recommend against the start method he proposes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.