Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite the multipart/form-data handling under FastCGI #15

Merged

Conversation

jonasmalacofilho
Copy link
Member

@jonasmalacofilho jonasmalacofilho commented Nov 2, 2017

This reimplements and optimizes the ClientFcgi handling of CQueryMultipart messages in FastCGI mode,

  • processing multipart/form-data lazily and incrementally,
  • keeping the number and size of string allocations to a minimum,
  • and placing it into a separate module.

FastCGI multipart/form-data will be processed until the first STDIN message is received, at which point control will be handled to the application. Processing will resume after the application calls neko.Web.parseMultipart.

This follows the behavior of mod_neko and mod_tora.

To compensate for the increased complexity, there are also some unit tests.

This PR also incorporates and closes #10; for the record, lower-case headers seem to be sent by some React-Native apps.

Motivation

This PR solves some issues my team and I experienced on a production service that used tora + FastCGI to handle uploads of reasonably sized files.

In comparison with current tora, our requirements were to:

  • don't error (don't trigger the OOM killer and don't timeout) on large requests
  • reduce response times for file uploads
  • check for authentication before processing possibly large amounts of data
  • allow the application to more meaningfully measure its response time
  • reduce heap size and fragmentation

Real world test results

A 7.9 MiB POST outlines the improvements:

metric before after
time in application space 67 ms 145 ms
time to first byte of response 626 ms 148 ms
neko.vm.Gc.stats() 72 MiB (65 MiB used) 6 MiB (5 MiB used)

but a larger 39 MiB POST makes this PR essential:

metric before after
time in application space 307 ms 667 ms
time to first byte of response 12780 ms 676 ms
neko.vm.Gc.stats() 3970 MiB (3000 MiB used) 6 MiB (6 MiB used)

And file sizes greater than 100 MiB generally caused the old (master) tora to be terminated by the OOM killer (after nearly exhausting all available memory + swap); in the rare case the request completed, Nginx or the client would have already timed out.

Note: the time spent in application space increases with this patch, but that is actually the desired: we're no longer pre-processing a lot of data before starting and handling control to the application.


Metrics:

  • time in application space: time from start to end of the cached entry point of the application (i.e. neko module loaded by tora)
  • time to first byte of response: response (first byte) time perceived by local client (Chrome 61)
  • heap/GC statistics from neko.vm.Gc.stats(), pretty printed

Test case:

  • actual route from a real world project
  • uploads of reasonably sized files with multipart/form-data
  • tora application hashes and writes (to disk) file content
  • application version: (closed source) jonasmalacofilho/proto-ead@a51e1ec, "dev" build

Configuration:

 - handle large inputs
 - don't process multipart data ahead of time
 - reduce the number and size of string allocations
 - add unit tests
@jonasmalacofilho jonasmalacofilho force-pushed the efficent-fcgi-multipart branch from 9649ca0 to 3506369 Compare March 28, 2019 05:42
@jonasmalacofilho
Copy link
Member Author

@ncannasse do you have any objections against this being merged?

I'm waiting for the tests to run, as I just did a rebase, but given that this code has been running in a couple of production systems without issues for almost a year and half, I would like to merge it.

By the way, thanks for merging #16, I was just waiting for the tests to complete.

@ncannasse
Copy link
Member

Hi @jonasmalacofilho , I no longer use Tora but it seems you did the right thing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants