varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.56k stars 366 forks source link

Move decision on fetch_chunksize to the storage engine #4056

Closed nigoroll closed 4 months ago

nigoroll commented 4 months ago

For chunked encoding, we do not know how big the object is ultimately going to be, so VFP_GetStorage() called ObjGetSpace() with the fetch_chunksize parameter in this case.

Yet which size is best might differ for different storage engines, and having the information that the caller does not know the final size might be relevant. Storage engines could guess that if a request came in for fetch_chunksize that this might be the "chunked" case, but that heuristic would be wrong for Objects of just that size advertised via Content-Length.

So this patch takes the guesswork out of the game by just passing the magic 0 value down to the storage engine to mean "give me some good chunk of bytes, I do not know how much I am going to need".


For context: Over the past two years, this particular aspect has given me a hard time again and again, it was the main driver for an unnecessarily complicated heuristic and is related to another issue which I just fixed.

dridi commented 4 months ago

The sanitizer error looks unrelated to this change, but maybe worth looking at:

*    diag  0.0 =================================================================
*    diag  0.0 ==37898==ERROR: LeakSanitizer: detected memory leaks
*    diag  0.0 
*    diag  0.0 Direct leak of 1376 byte(s) in 1 object(s) allocated from:
*    diag  0.0     #0 0x7fc1038343b7 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
*    diag  0.0     #1 0x557839acf3b7 in http_process ../../../../bin/varnishtest/vtc_http.c:1826
*    diag  0.0     #2 0x557839b4c688 in sess_process ../../../../bin/varnishtest/vtc_sess.c:113
*    diag  0.0     #3 0x557839b4cfb6 in sess_thread ../../../../bin/varnishtest/vtc_sess.c:140
*    diag  0.0     #4 0x7fc1029d0133  (/lib/x86_64-linux-gnu/libc.so.6+0x89133)
*    diag  0.0 
*    diag  0.0 Indirect leak of 2097152 byte(s) in 1 object(s) allocated from:
*    diag  0.0     #0 0x7fc1038349cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
*    diag  0.0     #1 0x557839acff35 in http_process ../../../../bin/varnishtest/vtc_http.c:1847
*    diag  0.0     #2 0x557839b4c688 in sess_process ../../../../bin/varnishtest/vtc_sess.c:113
*    diag  0.0     #3 0x557839b4cfb6 in sess_thread ../../../../bin/varnishtest/vtc_sess.c:140
*    diag  0.0     #4 0x7fc1029d0133  (/lib/x86_64-linux-gnu/libc.so.6+0x89133)
*    diag  0.0 
*    diag  0.0 Indirect leak of 64 byte(s) in 1 object(s) allocated from:
*    diag  0.0     #0 0x7fc1038349cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
*    diag  0.0     #1 0x557839ad05ba in http_process ../../../../bin/varnishtest/vtc_http.c:1858
*    diag  0.0     #2 0x557839b4c688 in sess_process ../../../../bin/varnishtest/vtc_sess.c:113
*    diag  0.0     #3 0x557839b4cfb6 in sess_thread ../../../../bin/varnishtest/vtc_sess.c:140
*    diag  0.0     #4 0x7fc1029d0133  (/lib/x86_64-linux-gnu/libc.so.6+0x89133)
*    diag  0.0 
*    diag  0.0 Indirect leak of 40 byte(s) in 1 object(s) allocated from:
*    diag  0.0     #0 0x7fc1038349cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
*    diag  0.0     #1 0x7fc10353c451 in VSB_new_auto ../../../../lib/libvarnish/vsb.c:230
*    diag  0.0     #2 0x557839ad03f5 in http_process ../../../../bin/varnishtest/vtc_http.c:1853
*    diag  0.0     #3 0x557839b4c688 in sess_process ../../../../bin/varnishtest/vtc_sess.c:113
*    diag  0.0     #4 0x557839b4cfb6 in sess_thread ../../../../bin/varnishtest/vtc_sess.c:140
*    diag  0.0     #5 0x7fc1029d0133  (/lib/x86_64-linux-gnu/libc.so.6+0x89133)
*    diag  0.0 
*    diag  0.0 Indirect leak of 16 byte(s) in 1 object(s) allocated from:
*    diag  0.0     #0 0x7fc1038349cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
*    diag  0.0     #1 0x557839ad06ed in http_process ../../../../bin/varnishtest/vtc_http.c:1861
*    diag  0.0     #2 0x557839b4c688 in sess_process ../../../../bin/varnishtest/vtc_sess.c:113
*    diag  0.0     #3 0x557839b4cfb6 in sess_thread ../../../../bin/varnishtest/vtc_sess.c:140
*    diag  0.0     #4 0x7fc1029d0133  (/lib/x86_64-linux-gnu/libc.so.6+0x89133)
*    diag  0.0 
*    diag  0.0 Indirect leak of 16 byte(s) in 1 object(s) allocated from:
*    diag  0.0     #0 0x7fc1038349cf in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:69
*    diag  0.0     #1 0x7fc10353c196 in VSB_newbuf ../../../../lib/libvarnish/vsb.c:204
*    diag  0.0     #2 0x7fc10353c481 in VSB_new_auto ../../../../lib/libvarnish/vsb.c:233
*    diag  0.0     #3 0x557839ad03f5 in http_process ../../../../bin/varnishtest/vtc_http.c:1853
*    diag  0.0     #4 0x557839b4c688 in sess_process ../../../../bin/varnishtest/vtc_sess.c:113
*    diag  0.0     #5 0x557839b4cfb6 in sess_thread ../../../../bin/varnishtest/vtc_sess.c:140
*    diag  0.0     #6 0x7fc1029d0133  (/lib/x86_64-linux-gnu/libc.so.6+0x89133)
bsdphk commented 4 months ago

OK with me.

nigoroll commented 4 months ago

The sanitizer error looks unrelated to this change

Looks like this was only concerning vtc? Also, we have some outstanding issues on coverity. Do you want to have a look?