Odin icon indicating copy to clipboard operation
Odin copied to clipboard

Published 20 hours ago • verified publisher icon odin-lang

Add `core:io` test suite

Open Feoramund opened this issue 1 year ago • 16 comments

This started out with me wanting io.Section_Reader to work as I expected it to: to work off of the supplied base offset. So, I wrote a test suite for core:io and found a handful of bugs with various streams and platforms.

The only thing I'm not certain of is if bytes.Buffer should have the behavior it has. It's not congruent to any other stream: its Size decreases 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 the Size shouldn'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 7663a2036aa121900b6e13c60e1dd7f5297f4f5e.

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.Stream and 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 comprehensive io testing.

Right now I have tests for:

  • bytes.Reader
  • bytes.Buffer as a stream
  • io.Limited_Reader
  • io.Section_Reader
  • strings.Builder as a stream
  • os.open file descriptor streams

I 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 os2 much at all, save for checking which interfaces had missing query responses, so if I need to move some of the os-affected functionality into that package, I can do so.

Feoramund avatar Aug 19 '24 23:08 Feoramund

io:core...

flysand7 avatar Aug 20 '24 02:08 flysand7

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

flysand7 avatar Aug 20 '24 02:08 flysand7

It looks like Darwin, despite using the POSIX interface on os2 as far as I can tell, is returning no error when it should error after using a closed file descriptor. NetBSD and FreeBSD use the same interface, and they're having no issue. Mysterious.

I think the os2 interface for Windows might be causing a stack overflow somewhere, as it's consistently exiting with -1073741571 / 0xC000_00FD. I don't have the means to probe into this more deeply at the moment.

Feoramund avatar Aug 22 '24 23:08 Feoramund

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 posix.fstat(0) (because the file struct was zero'd) instead of the actual file descriptor you had before calling close. I am guessing fstat on file descriptor 0 is fine on darwin but not on the bsds, but that's just a guess.

This is not something you should test though, a use-after-free means all bets are off.

By the way, you can do -sanitize:address and it will yell at you because of the use-after-free (at least it did for me).

laytan avatar Aug 23 '24 17:08 laytan

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 posix.fstat(0) (because the file struct was zero'd) instead of the actual file descriptor you had before calling close. I am guessing fstat on file descriptor 0 is fine on darwin but not on the bsds, but that's just a guess.

This is not something you should test though, a use-after-free means all bets are off.

I see now. Since we're deleting the File_Impl itself, and that's what's stored on the io.Stream, there's no way we can safely check if it's still usable either. I wasn't aware the API wasn't designed to not handle use-after-close situations, but that's understandable now. I could add some comments around this and remove the various after-close tests. In that case, do we not need an Invalid_Stream error? I could remove that as well.

By the way, you can do -sanitize:address and it will yell at you because of the use-after-free (at least it did for me).

I know of this flag but don't use it enough, and it did indeed raise a warning when I tried it.

Feoramund avatar Aug 23 '24 19:08 Feoramund

In that case, do we not need an Invalid_Stream error?

I don't think it is needed, but it could be hit when somebody does os2.fd(file) to get the underlying OSs handle and then manually closes that through that OS API. If that just comes up as unknown I wouldn't complain, but your call.

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 close(fd), if anywhere else you do something that generates a new file descriptor, and then try using the fd again, you would be doing operations on that newly created file descriptor and won't get any errors. It is just the easy case where you close and then use right away that is handled.

laytan avatar Aug 23 '24 20:08 laytan

I've rebased the Invalid_Stream error out of this PR, deciding instead to document os2.close explicitly that using a file stream in any way after closing it is like a use-after-free bug, therefore no more testing post-Close cases for any io.Streams.

I had to fix a few bugs with the os2 Windows file implementation, but it's all working fine now.

Feoramund avatar Aug 26 '24 05:08 Feoramund

Looks good to me!

I just remembered that bufio also has some streams but we can also add those in the future.

laytan avatar Aug 26 '24 16:08 laytan

I just remembered that bufio also has some streams but we can also add those in the future.

I've added bufio tests now. Setting that up revealed more inconsistencies in the stream API, so now all streams should return 0, nil if they're passed an empty slice in the read/write procs.

It also looks like this has revealed an issue with EOF detection in os2 Windows. I suspect that the note in os Windows about a successful read meaning a possible EOF might be relevant, but I don't have time to investigate this at the moment.

Feoramund avatar Aug 27 '24 23:08 Feoramund

It also looks like this has revealed an issue with EOF detection in os2 Windows. I suspect that the note in os Windows about a successful read meaning a possible EOF might be relevant, but I don't have time to investigate this at the moment.

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?

laytan avatar Aug 28 '24 15:08 laytan

Hmm looks like ubuntu CI is deadlocking now for some reason, ran it twice and it deadlocked at the same spot 🤔

laytan avatar Aug 28 '24 15:08 laytan

Looks like a deadlock in os2/heap_linux.odin:602, only on running the tests with multiple threads, looks like a bug in that allocator.

laytan avatar Aug 28 '24 17:08 laytan

-sanitize:thread is not happy with the allocator:

WARNING: ThreadSanitizer: data race (pid=12498)
  Read of size 8 at 0x0000019553b0 by thread T1:
    #0 os2._region_retrieve_with_space-5869 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:515:8 (io+0x553368) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 os2.heap_alloc-5760 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:261:20 (io+0x52c93c) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 os2._heap_allocator_proc-5747.aligned_alloc-0 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:161:18 (io+0x581123) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 os2._heap_allocator_proc-5747 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:194:3 (io+0x51a940) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 os2.heap_allocator_proc /home/laytan/third-party/odin/core/os/os2/heap.odin:18:2 (io+0x4df03d) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 runtime.mem_alloc_bytes /home/laytan/third-party/odin/base/runtime/internal.odin:127:2 (io+0x501c44) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #6 runtime.new_aligned-29033 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:278:2 (io+0x5319a0) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #7 runtime.new-28716 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:274:2 (io+0x50068f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #8 os2._new_file-5445 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:102:2 (io+0x4ff294) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #9 os2._open-5435 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:98:2 (io+0x4f5bdf) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #10 os2.open /home/laytan/third-party/odin/core/os/os2/file.odin:104:2 (io+0x5003ea) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #11 test_core_io.test_bufio_buffered_read_writer /home/laytan/third-party/odin/tests/core/io/test_core_io.odin:693:2 (io+0x55a45a) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #12 testing.run_test_task-792 /home/laytan/third-party/odin/core/testing/runner.odin:148:2 (io+0x50d2df) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #13 thread.pool_do_work /home/laytan/third-party/odin/core/thread/thread_pool.odin:312:3 (io+0x52e090) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #14 thread.pool_thread_runner-4844 /home/laytan/third-party/odin/core/thread/thread_pool.odin:62:4 (io+0x4f4a75) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #15 thread._create-4660.__unix_thread_entry_proc-0 /home/laytan/third-party/odin/core/thread/thread_unix.odin:60:4 (io+0x57e333) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

  Previous atomic write of size 8 at 0x0000019553b0 by thread T2:
    #0 os2._new_region-5826 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:352:6 (io+0x533fc1) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 os2._region_retrieve_with_space-5869 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:537:2 (io+0x553592) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 os2.heap_alloc-5760 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:261:20 (io+0x52c93c) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 os2._heap_allocator_proc-5747.aligned_alloc-0 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:161:18 (io+0x581123) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 os2._heap_allocator_proc-5747 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:194:3 (io+0x51a940) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 os2.heap_allocator_proc /home/laytan/third-party/odin/core/os/os2/heap.odin:18:2 (io+0x4df03d) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #6 runtime.mem_alloc_bytes /home/laytan/third-party/odin/base/runtime/internal.odin:127:2 (io+0x501c44) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #7 runtime.new_aligned-29033 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:278:2 (io+0x5319a0) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #8 runtime.new-28716 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:274:2 (io+0x50068f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #9 os2._new_file-5445 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:102:2 (io+0x4ff294) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #10 os2._open-5435 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:98:2 (io+0x4f5bdf) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #11 os2.open /home/laytan/third-party/odin/core/os/os2/file.odin:104:2 (io+0x5003ea) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #12 test_core_io.test_os2_file_stream /home/laytan/third-party/odin/tests/core/io/test_core_io.odin:592:2 (io+0x556cb1) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #13 testing.run_test_task-792 /home/laytan/third-party/odin/core/testing/runner.odin:148:2 (io+0x50d2df) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #14 thread.pool_do_work /home/laytan/third-party/odin/core/thread/thread_pool.odin:312:3 (io+0x52e090) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #15 thread.pool_thread_runner-4844 /home/laytan/third-party/odin/core/thread/thread_pool.odin:62:4 (io+0x4f4a75) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #16 thread._create-4660.__unix_thread_entry_proc-0 /home/laytan/third-party/odin/core/thread/thread_unix.odin:60:4 (io+0x57e333) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

  Location is global 'os2.global_regions-5637' of size 8 at 0x0000019553b0 (io+0x19553b0)

  Thread T1 (tid=12500, running) created by main thread at:
    #0 pthread_create <null> (io+0x45d7eb) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 thread._create-4660 /home/laytan/third-party/odin/core/thread/thread_unix.odin:118:2 (io+0x4f06ba) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 thread.create /home/laytan/third-party/odin/core/thread/thread.odin:105:2 (io+0x503b2a) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 thread.pool_init /home/laytan/third-party/odin/core/thread/thread_pool.odin:84:3 (io+0x4f8f8f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 testing.runner-817 /home/laytan/third-party/odin/core/testing/runner.odin:313:2 (io+0x51b632) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 main <null> (io+0x586903) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

  Thread T2 (tid=12501, running) created by main thread at:
    #0 pthread_create <null> (io+0x45d7eb) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 thread._create-4660 /home/laytan/third-party/odin/core/thread/thread_unix.odin:118:2 (io+0x4f06ba) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 thread.create /home/laytan/third-party/odin/core/thread/thread.odin:105:2 (io+0x503b2a) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 thread.pool_init /home/laytan/third-party/odin/core/thread/thread_pool.odin:84:3 (io+0x4f8f8f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 testing.runner-817 /home/laytan/third-party/odin/core/testing/runner.odin:313:2 (io+0x51b632) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 main <null> (io+0x586903) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

SUMMARY: ThreadSanitizer: data race /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:515:8 in os2._region_retrieve_with_space-5869
==================
==================
WARNING: ThreadSanitizer: data race (pid=12498)
  Read of size 2 at 0x7ffff6bac02a by thread T1:
    #0 os2._region_retrieve_with_space-5869 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:516:3 (io+0x5533d0) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 os2.heap_alloc-5760 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:261:20 (io+0x52c93c) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 os2._heap_allocator_proc-5747.aligned_alloc-0 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:161:18 (io+0x581123) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 os2._heap_allocator_proc-5747 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:194:3 (io+0x51a940) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 os2.heap_allocator_proc /home/laytan/third-party/odin/core/os/os2/heap.odin:18:2 (io+0x4df03d) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 runtime.mem_alloc_bytes /home/laytan/third-party/odin/base/runtime/internal.odin:127:2 (io+0x501c44) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #6 runtime.new_aligned-29033 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:278:2 (io+0x5319a0) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #7 runtime.new-28716 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:274:2 (io+0x50068f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #8 os2._new_file-5445 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:102:2 (io+0x4ff294) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #9 os2._open-5435 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:98:2 (io+0x4f5bdf) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #10 os2.open /home/laytan/third-party/odin/core/os/os2/file.odin:104:2 (io+0x5003ea) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #11 test_core_io.test_bufio_buffered_read_writer /home/laytan/third-party/odin/tests/core/io/test_core_io.odin:693:2 (io+0x55a45a) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #12 testing.run_test_task-792 /home/laytan/third-party/odin/core/testing/runner.odin:148:2 (io+0x50d2df) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #13 thread.pool_do_work /home/laytan/third-party/odin/core/thread/thread_pool.odin:312:3 (io+0x52e090) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #14 thread.pool_thread_runner-4844 /home/laytan/third-party/odin/core/thread/thread_pool.odin:62:4 (io+0x4f4a75) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #15 thread._create-4660.__unix_thread_entry_proc-0 /home/laytan/third-party/odin/core/thread/thread_unix.odin:60:4 (io+0x57e333) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

  Previous write of size 2 at 0x7ffff6bac02a by thread T2:
    #0 os2.heap_alloc-5760 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:283:32 (io+0x52cd2c) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 os2._heap_allocator_proc-5747.aligned_alloc-0 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:161:18 (io+0x581123) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 os2._heap_allocator_proc-5747 /home/laytan/third-party/odin/core/os/os2/heap_linux.odin:194:3 (io+0x51a940) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 os2.heap_allocator_proc /home/laytan/third-party/odin/core/os/os2/heap.odin:18:2 (io+0x4df03d) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 runtime.mem_alloc_bytes /home/laytan/third-party/odin/base/runtime/internal.odin:127:2 (io+0x501c44) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 runtime.make_aligned-26168 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:298:2 (io+0x535921) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #6 runtime.make_slice-25803 /home/laytan/third-party/odin/base/runtime/core_builtin.odin:312:2 (io+0x54e471) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #7 os2._read_link_cstr-5741 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:325:2 (io+0x52af92) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #8 os2._get_full_path-5604 /home/laytan/third-party/odin/core/os/os2/path_linux.odin:184:19 (io+0x5163b7) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #9 os2._new_file-5445 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:109:12 (io+0x4ff5e5) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #10 os2._open-5435 /home/laytan/third-party/odin/core/os/os2/file_linux.odin:98:2 (io+0x4f5bdf) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #11 os2.open /home/laytan/third-party/odin/core/os/os2/file.odin:104:2 (io+0x5003ea) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #12 test_core_io.test_os2_file_stream /home/laytan/third-party/odin/tests/core/io/test_core_io.odin:592:2 (io+0x556cb1) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #13 testing.run_test_task-792 /home/laytan/third-party/odin/core/testing/runner.odin:148:2 (io+0x50d2df) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #14 thread.pool_do_work /home/laytan/third-party/odin/core/thread/thread_pool.odin:312:3 (io+0x52e090) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #15 thread.pool_thread_runner-4844 /home/laytan/third-party/odin/core/thread/thread_pool.odin:62:4 (io+0x4f4a75) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #16 thread._create-4660.__unix_thread_entry_proc-0 /home/laytan/third-party/odin/core/thread/thread_unix.odin:60:4 (io+0x57e333) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

  Thread T1 (tid=12500, running) created by main thread at:
    #0 pthread_create <null> (io+0x45d7eb) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 thread._create-4660 /home/laytan/third-party/odin/core/thread/thread_unix.odin:118:2 (io+0x4f06ba) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 thread.create /home/laytan/third-party/odin/core/thread/thread.odin:105:2 (io+0x503b2a) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 thread.pool_init /home/laytan/third-party/odin/core/thread/thread_pool.odin:84:3 (io+0x4f8f8f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 testing.runner-817 /home/laytan/third-party/odin/core/testing/runner.odin:313:2 (io+0x51b632) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 main <null> (io+0x586903) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

  Thread T2 (tid=12501, running) created by main thread at:
    #0 pthread_create <null> (io+0x45d7eb) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #1 thread._create-4660 /home/laytan/third-party/odin/core/thread/thread_unix.odin:118:2 (io+0x4f06ba) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #2 thread.create /home/laytan/third-party/odin/core/thread/thread.odin:105:2 (io+0x503b2a) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #3 thread.pool_init /home/laytan/third-party/odin/core/thread/thread_pool.odin:84:3 (io+0x4f8f8f) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #4 testing.runner-817 /home/laytan/third-party/odin/core/testing/runner.odin:313:2 (io+0x51b632) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)
    #5 main <null> (io+0x586903) (BuildId: ad0ef22dbbcb61f9cfde8845c7e7c7f436b36459)

laytan avatar Aug 28 '24 17:08 laytan

I disabled the custom allocator in https://github.com/odin-lang/Odin/pull/4162 for now and rebased this.

laytan avatar Aug 28 '24 17:08 laytan

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 bytes.Buffer's odd behavior. If someone wanted to address that, that would be helpful.

Feoramund avatar Aug 28 '24 18:08 Feoramund

The only thing I'm not certain of is if bytes.Buffer should have the behavior it has. It's not congruent to any other stream: its Size decreases 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 the Size shouldn't decrease and the buffer deleting itself was unexpected to me.

EDIT: Though, I don't think I ever got any feedback on bytes.Buffer's odd behavior. If someone wanted to address that, that would be helpful.

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.

laytan avatar Aug 28 '24 18:08 laytan

I thought on this for a day. I think any major API change to bytes.Buffer would be out of scope of this PR, though I've considered that this sort of functionality could be split out into a different type of buffer or stream procedure, if there's a need for it.

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 bytes.Buffer is the only stream I could find that makes an entire copy of the buffer slice you give it in its _init proc, instead of directly using the slice (which makes sense given that it uses a dynamic array under the hood), but that too can be surprising in its own way (especially for very large slices under certain memory constraints).

It's possible there could be two different buffers for bytes, one dynamic and the other a slice, then two (or more?) stream procedures that could be selected when converting each buffer to a stream which either limits or expands its functionality depending on whether the user wants a buffer that decreases in size or self-destructs. Thoughts for another day, at least.

Feoramund avatar Aug 29 '24 19:08 Feoramund