Discord.Net icon indicating copy to clipboard operation
Discord.Net copied to clipboard

[Bug]: SendFileAsync Dispose()s the provided Stream

Open HLSSLenda opened this issue 2 years ago • 2 comments

Check The Docs

  • [X] I double checked the docs and couldn't find any useful information.

Verify Issue Source

  • [X] I verified the issue was caused by Discord.Net.

Check your intents

  • [X] I double checked that I have the required intents.

Description

If I open a Stream and I call RespondWithFileAsync or SendFileAsync, the Stream gets disposed after Discord.NET sends the message. If the caller allocates or gets somehow a Stream, the caller should be responsible for calling Dispose on the stream, not Discord.NET itself.

FileAsync wraps the stream under a FileAttachment inside a using block, disposing it and the Stream itself at the end of the method (not familiarized enough with Discord.NET code to do a pull request though)

Version

3.13.0

Working Version

No response

Logs

System.Net.Http.HttpRequestException: Error while copying content to a stream.
 ---> System.ObjectDisposedException: Cannot access a closed Stream.

Sample

// inside a command
const string NAME = "i_dont_have_creativity_for_a_png_name.png";
using Stream myImage = File.OpenRead(NAME);
await RespondWithFileAsync(myImage, NAME, "Look at my image!");
await FollowupWithFileAsync(myImage, NAME, "LOOK AGAIN, YOU'RE NOT LOOKING ENOUGH"); // ObjectDisposedException here

Packages

null

Environment

  • OS: Debian GNU/Linux 12
  • Architecture: x64
  • SDK: 8.0.100

HLSSLenda avatar Dec 16 '23 19:12 HLSSLenda

I'm not completely sure, but your code

using Stream myImage = File.OpenRead(NAME);

is the problem. "using" indicates that it should dispose of itself after the first use. Hope this helps!

DutchSlav avatar Dec 27 '23 20:12 DutchSlav

"using" indicates that it should dispose of itself after the first use.

Nope. using disposes when it exits the scope.

{
    using var foo = Bar();
    // use foo
    // use foo again?
}

is equivalent to

using (var foo = Bar())
{
    // use foo
    // use foo again?
}

is equivalent to

var foo = Bar();
try
{
    // use foo
    // use foo again?
}
finally
{
    foo?.Dispose();
}

You may use the disposable object as many times as you wish within the using scope. Of course, many stream types are not seekable, so using it multiple times often does not make sense. Since you know you have a FileStream that is seekable, you'd normally be expected to seek back to the beginning before reuse. That said Discord.Net does internally try to seek back to 0 if possible.


HLSSLenda is correct that the culprit is https://github.com/discord-net/Discord.Net/blob/8227d70b8610ccc7e2dbe2ba4fceba778666dfa4/src/Discord.Net.WebSocket/Entities/Interaction/SocketInteraction.cs#L190, which wraps the provided stream inside a FileAttachment with its own using. You could probably get around this by constructing your own FileAttachment and passing it in via RespondWithFileAsync(FileAttachment attachment, ..., but this also bypasses the seek so you must also attachment.Stream.Position = 0 in between uses of it unless you actually expect the stream to provide fresh data.

And yea the library probably shouldn't be disposing streams it receives but does not own. Perhaps FileAttachment could be extended to work in the same way StreamWriter etc. do, with a leaveOpen param that indicates it should not dispose the underlying stream when it is disposed itself? Then RespondWithFileAsync could pass that when it constructs the attachment wrapper.

BobVul avatar Dec 27 '23 21:12 BobVul