Add core:io test suite
#4112
Add core:io test suite
#4112
Conversation
|
|
|
That said, a test suite for I/O streams sounds nice. It is an abstraction, and testing for surface-level assumptions that one might make for such streams is good, since the implementations of these streams typically aren't straightforward |
Feoramund
force-pushed
the
fix-test-io-issues
branch
from
b90eed1
to
00a3b4d
Compare
|
It looks like Darwin, despite using the POSIX interface on I think the |
|
Closing a file in os2 does a little more, it also frees and zeros memory. You are effectively testing a use-after-free where all bets are off. I did put in a couple print statements, and the call to size ends up calling This is not something you should test though, a use-after-free means all bets are off. By the way, you can do |
I see now. Since we're deleting the
I know of this flag but don't use it enough, and it did indeed raise a warning when I tried it. |
I don't think it is needed, but it could be hit when somebody does As for not being designed to handle use after closing, I don't think that's really possible because like you said we have no idea if the pointer in the stream is still valid. IMO classifying that as a bug (basically a use-after-free) on the user's end is fine. Even with libc, doing |
Feoramund
force-pushed
the
fix-test-io-issues
branch
3 times, most recently
from
c979c29
to
ece3188
Compare
|
I've rebased the I had to fix a few bugs with the |
|
Looks good to me! I just remembered that |
Feoramund
force-pushed
the
fix-test-io-issues
branch
from
956e975
to
6d3e5f0
Compare
I've added It also looks like this has revealed an issue with EOF detection in |
You were right about that, os2 wasn't seeing that as EOF, I have committed a fix for this and also another test failure because we were doing os2.remove on a file that wasn't closed because the bufio.Read_Writer doesn't implement close. Really great to have these tests to make sure the streams all behave as expected! I believe this is ready to merge now? |
|
Hmm looks like ubuntu CI is deadlocking now for some reason, ran it twice and it deadlocked at the same spot 🤔 |
|
Looks like a deadlock in |
|
|
This should fix seeking from `.Start`, getting the `size`, and `read_at`. Also make the API consistent with the other `*_init` procs in `util.odin` by returning the `io.Reader`.
Makes the API like the other stream `init` procs.
This is consistent with the other stream `read` procs
Handles `EINVAL`, among other fixes.
These procedures must not modify the underlying file pointer.
Platforms: - FreeBSD - Haiku - Linux - NetBSD - OpenBSD
- Move `Seek`-related checks into OS-specific files for granularity. Platforms: - Darwin - FreeBSD - Haiku - Linux - NetBSD - OpenBSD
- Relax return value requirements on errors. Only the error is checked, as there are multiple conflicting return styles throughout the `os` API. Some return the error along with `0`, `-1`, or another value. This can be smoothed out later. - Test `os2` files now. - No longer test streams after closing; this is considered similar to a use-after-free in `os2`.
Make it explicit that using a stream after closing is like a use-after-free bug.
The previous code was jumping ahead by the specified offset, instead of getting the current offset.
laytan
force-pushed
the
fix-test-io-issues
branch
from
2b78a13
to
cca3852
Compare
|
I disabled the custom allocator in #4162 for now and rebased this. |
|
I had a glance over the changes, and it seems alright. I have nothing more to add, and I'd say we're ready to merge after a review. EDIT: Though, I don't think I ever got any feedback on |
I do not know why it is that way myself. The behavior makes "some" sense that it returns the size that is left to be read, destroying itself is a little weird to me too. |
|
I thought on this for a day. I think any major API change to The decrementing size and buffer self-destruct is something that needs to be explicitly documented at least, since this is the oddball of the streams. It's also worth noting that It's possible there could be two different buffers for |
This started out with me wanting
io.Section_Readerto work as I expected it to: to work off of the supplied base offset. So, I wrote a test suite forcore:ioand found a handful of bugs with various streams and platforms.The only thing I'm not certain of is if
bytes.Buffershould have the behavior it has. It's not congruent to any other stream: itsSizedecreases as it reads, and when it reaches zero while reading, it clears and resets the underlying buffer. I had to add some extra handling for it in the test suite. Could use some feedback on this one. I'm inclined to say theSizeshouldn't decrease and the buffer deleting itself was unexpected to me.It's worth noting up front that I made Windows behave like POSIX with regards to
read_at/write_at: these procedures will seek back to their point of origin after operation. This may not be what everyone expects, so I'm open to how this should work across the platforms. I had to fix a few platforms around that general issue with regards to 7663a20.Note: We don't test for Haiku and I don't have an installation, so my fix for that OS is based on my fix for NetBSD's. Same case to OpenBSD.
The test suite accepts any
io.Streamand runs a grab-bag of tests against whatever features it says it supports; a great benefit to having a feature query interface. Not all errors are tested for; it could be expanded, but it does a lot of sanity checking. A good first step towards more comprehensiveiotesting.Right now I have tests for:
bytes.Readerbytes.Bufferas a streamio.Limited_Readerio.Section_Readerstrings.Builderas a streamos.openfile descriptor streamsI think those are the basic ones, but if there's any more that need to be added, let me know.
Note2: I haven't looked into
os2much at all, save for checking which interfaces had missing query responses, so if I need to move some of theos-affected functionality into that package, I can do so.