Skip to content

Commit

Permalink
Actually don't dispose hawkfile stream after ReadAllBytes()
Browse files Browse the repository at this point in the history 
fixes a30c0ef
  • Loading branch information
@CasualPokePlayer
CasualPokePlayer committed Jan 10, 2025
1 parent a30c0ef commit 6336818
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/BizHawk.Common/HawkFile/HawkFile.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ public void Dispose()
/// <summary>attempts to read all the content from the file</summary>
public byte[] ReadAllBytes()
{
using var stream = GetStream();
var stream = GetStream();
using var ms = new MemoryStream((int) stream.Length);
stream.CopyTo(ms);
return ms.GetBuffer();
Expand Down

5 comments on commit 6336818

@YoshiRulz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're re-using it now, don't you need to Seek?

@CasualPokePlayer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes (or set Position to 0), although that is the same pattern our ReadAllBytes() Stream extension does (relying on caller). RomGame callsite sets Position to 0 beforehand.

@YoshiRulz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow, this isn't using Stream.ReadAllBytes, and neither is RomGame (for the rom at least).

@CasualPokePlayer
Copy link
Member Author

@CasualPokePlayer CasualPokePlayer commented on 6336818 Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream.ReadAllBytes has the pattern of also relying on caller to set Position correctly, just like HawkFile.ReadAllBytes does (stupid but didn't want to bother fixing it everywhere since everywhere obeys it).

The one callsite which this matters for HawkFiles, in RomGame, does set 0 for Position

BizHawk/src/BizHawk.Client.Common/RomGame.cs

Lines 96 to 97 in 6336818

stream.Position = 0;
var bytesRead = stream.Read(FileData, offset: 0, count: fileLength);

(edit: realizing this is not ReadAllBytes, it's just regular read, I'm dumb)

@YoshiRulz
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with c5a4ec9.

Please sign in to comment.