saviorand / lightbug_http

Simple and fast HTTP framework for Mojo! 🔥
MIT License
587 stars 37 forks source link

Using stringbuilder and switching to as_bytes instead of _buffer #16

Closed thatstoasty closed 8 months ago

thatstoasty commented 8 months ago

@saviorand Does this branch work for you? I forked from main and tried changing only a few places rather than doing a global replace. I also did not include usage of the Bytes struct yet. There were some issues with mixing how Bytes uses it's internal data (no null terminator) and how char pointers and mojo strings use their data (null terminator needed).

I'm hoping this separates the work of the Bytes struct refactor from usage of the string builder. This branch I was able to run locally and also using the ubuntu image in the docker setup of the repo.

saviorand commented 8 months ago

Thanks for this! I'm still able to reproduce the issue on my machine, but also just got it in a Github codespace, which is a Linux machine under the hood as far as I know. Here are the steps to reproduce in a codespace:

  1. Create a new codespace from this PR
  2. Install a fresh mojo version with curl -s https://get.modular.com | sh - modular auth and modular install mojo
  3. In http.mojo replace the following on line 260 to write the actual body with the Lightbug mascot as the response to the request:
    - # _ = builder.write(res.body())
    -    print(str(builder))
    + _ = builder.write(res.body_raw)
  4. Save, run mojo lightbug.🔥

Get the following error:

🔥🐝 Lightbug is listening on http://0.0.0.0:8080
Ready to accept connections...
[6772:6772:20240323,084704.210475:ERROR file_io_posix.cc:144] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq: No such file or directory (2)
[6772:6772:20240323,084704.210562:ERROR file_io_posix.cc:144] open /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq: No such file or directory (2)
Please submit a bug report to https://github.com/modularml/mojo/issues and include the crash backtrace along with all the relevant source codes.
Stack dump:
0.      Program arguments: mojo lightbug.\360\237[6772:6773:20240323,084704.211883:ERROR directory_reader_posix.cc:42] opendir /home/codespace/.modular/crashdb/attachments/e8a82bff-b754-4ade-90bd-b2979cfae68c: No such file or directory (2)
\224\245
 #0 0x000055cc82b0fe47 (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x62ae47)
 #1 0x000055cc82b0da1e (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x628a1e)
 #2 0x000055cc82b1051f (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x62b51f)
 #3 0x00007f6d5c0ac420 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x14420)
 #4 0x000055cc84e0fe48 (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x292ae48)
 #5 0x000055cc84e0fee5 (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x292aee5)
 #6 0x000055cc84e102e9 (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x292b2e9)
 #7 0x000055cc85576510 (/home/codespace/.modular/pkg/packages.modular.com_mojo/bin/mojo+0x3091510)
 #8 0x00007f6d59a3a1b5 KGEN_CompilerRT_AlignedAlloc (/home/codespace/.modular/pkg/packages.modular.com_mojo/lib/libKGENCompilerRTShared.so.19git+0x251b5)
 #9 0x00007f6d0c0018ad 
Segmentation fault (core dumped)
thatstoasty commented 8 months ago

Thanks! With that change, I'm able to replicate the error on my machine and in the ubuntu image as well. I'll play around with it to see what about the string builder isn't playing nicely with the response body.

thatstoasty commented 8 months ago

@saviorand Hopefully these latest changes work on your local and in codespaces now!

It seems like the internal vector for stringbuilder was filling up since I had initially set up Bytes to act like a statically sized container. But, I've updated it so it dynamically grows like []byte. I've also added Bytes().get_bytes() and Bytes().get_null_terminated_bytes() so you can call that to make a copy of the internal vector with or without the null terminator.

saviorand commented 8 months ago

Nice, it all works. Merging now! Hope this was also helpful to improve gojo. Thanks again for the contirbution

thatstoasty commented 8 months ago

Yes definitely! It revealed issues with buffer resizing and handling payloads larger than the default buffer size which I've been needing to test more.