Fix file handle leak in read #139
Conversation
|
The actual fix is onRead, and tests are about 19x longer. Apologies for the extra work loaded onto the actual fix 👼🏽 |
(Useful for _my_ tests, should not be upstreamed)
There was a problem hiding this comment.
Thanks @arielshaqed!
Just had a couple of questions, leaving the real review to @willscott of course 🙂
| } | ||
| t.mu.Lock() | ||
| defer t.mu.Unlock() | ||
| id := rand.Int63() |
There was a problem hiding this comment.
Nit: Isn't the filename sufficient?
And if not, why not use a UUID, or even an atomically incremented ID?
There was a problem hiding this comment.
Files can be open multiple times, and OpenFile can be called multiple time with the exact same arguments. We need a "unique" ID.
Obviously a more unique ID would be better. But this is a test. As long as the test opens fewer than 10 or so million files, we should be cool.
ObMeme
A leftover from my local testing.
|
Thanks! Sorry about that nasty leftover in go.mod . |
|
Thanks! 37 minutes to review an external PR has got to be a record... |
|
Thanks again for the catch and the test!
Put in tag & release v0.0.3 for pointing to if useful.
…--Will
On Thu, Feb 6, 2025 at 10:47 AM Ariel Shaqed (Scolnicov) < ***@***.***> wrote:
Thanks! 37 minutes to review an external PR has got to be a record...
—
Reply to this email directly, view it on GitHub
<#139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADIRJ2FNNSLGFSIOKXAXST2OM4VBAVCNFSM6AAAAABWTELG66VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMMZZGQ3DOMBRGY>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
No description provided.