Skip to content

Commit

Permalink
Proper closing of resources in MessageAction
Browse files Browse the repository at this point in the history
Added the following methods

* clearFiles(Consumer)
* clearFiles(BiConsumer)

This should allow users to close any of their provided resources,
additionally clearFiles() will now close any opened FileInputStreams!

I've added a note to the class-level documentation that the execution of MessageAction closes all provided resources.
You cannot send the same resource twice without copying it first!

closes #660
  • Loading branch information
MinnDevelopment committed Apr 14, 2018
1 parent aec0f8d commit b8e7e4e
Showing 1 changed file with 100 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@

import javax.annotation.CheckReturnValue;
import java.io.*;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.*;
import java.util.function.BiConsumer;
import java.util.function.Consumer;

/**
* Extension of a default {@link net.dv8tion.jda.core.requests.RestAction RestAction}
Expand All @@ -49,6 +49,10 @@
* <br>This can be used to remove existing embeds from a message:
* <br>{@code message.editMessage("This message had an embed").override(true).queue()}
*
* <p><u>When this RestAction has been executed all provided files will be closed.
* If you decide not to execute this action, you should call {@link #clearFiles()} to free resources.</u>
* <br>Note that the garbage collector also frees opened file streams when it finalizes the stream object.
*
* <h1>Example</h1>
* <pre><code>
* {@literal @Override}
Expand All @@ -70,6 +74,7 @@ public class MessageAction extends RestAction<Message> implements Appendable
{
private static final String CONTENT_TOO_BIG = String.format("A message may not exceed %d characters. Please limit your input!", Message.MAX_CONTENT_LENGTH);
protected final Map<String, InputStream> files = new HashMap<>();
protected final Set<InputStream> ownedResources = new HashSet<>();
protected final StringBuilder content;
protected final MessageChannel channel;
protected MessageEmbed embed = null;
Expand Down Expand Up @@ -337,6 +342,7 @@ public MessageAction appendFormat(final String format, final Object... args)

/**
* Adds the provided {@link java.io.InputStream InputStream} as file data.
* <br><u>The stream will be closed upon execution!</u>
*
* <p>To reset all files use {@link #clearFiles()}
*
Expand Down Expand Up @@ -407,9 +413,7 @@ public MessageAction addFile(final byte[] data, final String name)

/**
* Adds the provided {@link java.io.File File} as file data.
* <br>Shortcut for {@link #addFile(java.io.File, String) addFile(file, file.getName())}
*
* <p>To reset all files use {@link #clearFiles()}
* <br>Shortcut for {@link #addFile(java.io.File, String) addFile(file, file.getName())} with the same side-effects.
*
* @param file
* The File that will be interpreted as file data
Expand Down Expand Up @@ -438,6 +442,7 @@ public MessageAction addFile(final File file)
* Adds the provided {@link java.io.File File} as file data.
*
* <p>To reset all files use {@link #clearFiles()}
* <br><u>This method opens a {@link java.io.FileInputStream FileInputStream} which will be closed by executing this action or using {@link #clearFiles()}!</u>
*
* @param file
* The File that will be interpreted as file data
Expand Down Expand Up @@ -470,7 +475,9 @@ public MessageAction addFile(final File file, final String name)
Checks.check(file.length() <= maxSize, "File may not exceed the maximum file length of %d bytes!", maxSize);
try
{
return addFile(new FileInputStream(file), name);
FileInputStream data = new FileInputStream(file);
ownedResources.add(data);
return addFile(data, name);
}
catch (FileNotFoundException e)
{
Expand All @@ -480,13 +487,69 @@ public MessageAction addFile(final File file, final String name)

/**
* Clears all previously added files
* <br>And closes {@code FileInputStreams} generated by {@link #addFile(File, String)}.
* <br>To close all stream (including ones given by {@link #addFile(InputStream, String)}) use {@link #clearFiles(Consumer)}.
*
* @return Updated MessageAction for chaining convenience
*
* @see #clearFiles(Consumer)
* @see #clearFiles(BiConsumer)
*/
@CheckReturnValue
public MessageAction clearFiles()
{
files.clear();
clearResources();
return this;
}

/**
* Clears all previously added files
*
* @param finalizer
* BiConsumer useful to <b>close</b> remaining resources,
* the consumer will receive the name as a string parameter and the resource as {@code InputStream}.
*
* @return Updated MessageAction for chaining convenience
*
* @see java.io.Closeable
*/
@CheckReturnValue
public MessageAction clearFiles(BiConsumer<String, InputStream> finalizer)
{
Checks.notNull(finalizer, "Finalizer");
for (Iterator<Map.Entry<String, InputStream>> it = files.entrySet().iterator(); it.hasNext();)
{
Map.Entry<String, InputStream> entry = it.next();
finalizer.accept(entry.getKey(), entry.getValue());
it.remove();
}
clearResources();
return this;
}

/**
* Clears all previously added files
* <br>The {@link #clearFiles(BiConsumer)} version provides the resource name for more selective operations.
*
* @param finalizer
* Consumer useful to <b>close</b> remaining resources,
* the consumer will receive only the resource in the form of an {@code InputStream}
*
* @return Updated MessageAction for chaining convenience
*
* @see java.io.Closeable
*/
@CheckReturnValue
public MessageAction clearFiles(Consumer<InputStream> finalizer)
{
Checks.notNull(finalizer, "Finalizer");
for (Iterator<InputStream> it = files.values().iterator(); it.hasNext(); )
{
finalizer.accept(it.next());
it.remove();
}
clearResources();
return this;
}

Expand All @@ -505,6 +568,23 @@ public MessageAction override(final boolean bool)
return this;
}

private void clearResources()
{
for (InputStream ownedResource : ownedResources)
{
try
{
ownedResource.close();
}
catch (IOException ex)
{
if (!ex.getMessage().toLowerCase().contains("closed"))
LOG.error("Encountered IOException trying to close owned resource", ex);
}
}
ownedResources.clear();
}

protected RequestBody asMultipart()
{
final MultipartBody.Builder builder = new MultipartBody.Builder().setType(MultipartBody.FORM);
Expand All @@ -517,6 +597,9 @@ protected RequestBody asMultipart()
}
if (!isEmpty())
builder.addFormDataPart("payload_json", getJSON().toString());
// clear remaining resources, they will be closed after being sent
files.clear();
ownedResources.clear();
return builder.build();
}

Expand Down Expand Up @@ -607,4 +690,14 @@ protected void handleResponse(Response response, Request<Message> request)
else
request.onFailure(response);
}

@Override
@Deprecated /* If this was in JDK9 we would be using java.lang.ref.Cleaner instead! */
protected void finalize()
{
if (ownedResources.isEmpty())
return;
LOG.warn("Found unclosed resources in MessageAction instance, closing on finalization step!");
clearResources();
}
}

0 comments on commit b8e7e4e

Please sign in to comment.