floooh / sokol

minimal cross-platform standalone C headers
https://floooh.github.io/sokol-html5
zlib License
6.53k stars 467 forks source link

Sokol Fetch Http response 200 is reported as an error #1062

Open WillisMedwell opened 3 weeks ago

WillisMedwell commented 3 weeks ago

Got errors flagged on the callback despite double checking and I have recieved a http success reponse code of 200.

         .callback = [](const sfetch_response_t* response){
                io_file_reader::M* members_ptr = nullptr;
                std::memcpy(&members_ptr, response->user_data, sizeof(members_ptr));

                if(response->finished && !response->failed) {
                    std::vector<unsigned char> contents(response->data.size);
                    std::copy_n((unsigned char*)response->data.ptr, response->data.size, contents.begin());
                    members_ptr->file_promises[response->handle.id].set_value(std::move(contents));
                    --(members_ptr->amount_queued_per_channel[response->channel]);
                }   
                else {
                    constexpr static std::string_view error_code_msg[] = 
                    {
                        [(int)SFETCH_ERROR_NO_ERROR] = "SFETCH_ERROR_NO_ERROR",
                        [(int)SFETCH_ERROR_FILE_NOT_FOUND] = "SFETCH_ERROR_FILE_NOT_FOUND",
                        [(int)SFETCH_ERROR_NO_BUFFER] = "SFETCH_ERROR_NO_BUFFER",
                        [(int)SFETCH_ERROR_BUFFER_TOO_SMALL] = "SFETCH_ERROR_BUFFER_TOO_SMALL",
                        [(int)SFETCH_ERROR_UNEXPECTED_EOF] = "SFETCH_ERROR_UNEXPECTED_EOF",
                        [(int)SFETCH_ERROR_INVALID_HTTP_STATUS] = "SFETCH_ERROR_INVALID_HTTP_STATUS",
                        [(int)SFETCH_ERROR_CANCELLED] = "SFETCH_ERROR_CANCELLED",
                    };
                    std::println(stderr, "Failed to load file, the response error code was: {} -> {}", (int)response->error_code, error_code_msg[(int)response->error_code]);
                }
            },

image and my debug print from sokol_fetch says its 200 so it should be g but it isn't for some reason?

EMSCRIPTEN_KEEPALIVE void _sfetch_emsc_failed_http_status(uint32_t slot_id, uint32_t http_status) {
    _sfetch_t* ctx = _sfetch_ctx();
    if (ctx && ctx->valid) {
        _sfetch_item_t* item = _sfetch_pool_item_lookup(&ctx->pool, slot_id);
        if (item) {
            if (http_status == 404) {
                item->thread.error_code = SFETCH_ERROR_FILE_NOT_FOUND;
            }
            else {
                fprintf(stderr, "%i", http_status);
                item->thread.error_code = SFETCH_ERROR_INVALID_HTTP_STATUS;
            }
            item->thread.failed = true;
            item->thread.finished = true;
            _sfetch_ring_enqueue(&ctx->chn[item->channel].user_outgoing, slot_id);
        }
    }
}

It seems like an easy change to just add a check to see if the http_status is between 200-209 and not report it as an error? I could do that?

floooh commented 2 weeks ago

Agreed, only checking for status code 200 is too narrow, however:

I suspect the problematic code is this:

https://github.com/floooh/sokol/blob/c54523c078e481d3084fa0b4630d2ce3d3e1e74f/sokol_fetch.h#L2225-L2239

...since the other location where __sfetch_emsc_failed_http_status is called wouldn't trigger an error on a HTTP status code 200:

https://github.com/floooh/sokol/blob/c54523c078e481d3084fa0b4630d2ce3d3e1e74f/sokol_fetch.h#L2200-L2207

...it might be about the need_range_request boolean. That is indeed confusing since the HTTP status code is entirely alright, but the reported error says it isn't, so at the very least there should be a better error for that case.

Are you trying to stream data in chunks?

E.g. I'd like to know more about the situation first. Are you able to reproduce the problem on your side and could check the Network tab in the browser what the requests and responses are going back and forth? If yes can you paste those here?

WillisMedwell commented 2 weeks ago
floooh commented 2 weeks ago

Hmm ok, the callstack confirms that it's coming out of this function:

https://github.com/floooh/sokol/blob/c54523c078e481d3084fa0b4630d2ce3d3e1e74f/sokol_fetch.h#L2213-L2242

...and it looks like it is related to 'range requests' (e.g. not reading the entire file at once, but in smaller chunks).

I seem to remember what's up: When sending a range-request to only read part of the data, I'm expecting that a 206 Partial Content HTTP status is returned, but instead of 200 is returned. If anything, the error reporting should be better.

A couple of questions:

Can you try to simply not set the the chunk-size to anything? E.g. just keep it zero-initialized or set it explicitly to zero (I realize though that = {} should do just that - but the error you're seeing seems to indicate that chunk_size is not zero).

...also, regardless of whether it's an actual sokol_fetch.h bug or not, I should at least do something about the confusing error that's reported when a chunk_size is set but the server doesn't return with a 206 status code.

floooh commented 2 weeks ago

PS: can you paste your sfetch_send() call with the sfetch_request_t content, and the size of the target buffer (e.g. sfetch_request_t.buffer.size) versus the file size?

WillisMedwell commented 2 weeks ago

Just changed it so the chunk_size was explicitly zero and it fixed the problem.

        const auto request = sfetch_request_t {
            .channel = channel,
            .path = path_buffer, 
            .callback = [](const sfetch_response_t* response){
                io_file_reader::M* members_ptr = nullptr;
                std::memcpy(&members_ptr, response->user_data, sizeof(members_ptr));

                if(response->finished && !response->failed)  {
                    std::vector<unsigned char> contents(response->data.size);
                    std::copy_n((unsigned char*)response->data.ptr, response->data.size, contents.begin());
                    members_ptr->file_promises[response->handle.id].set_value(std::move(contents));
                    --(members_ptr->amount_queued_per_channel[response->channel]);
                }   
                else {
                    constexpr static std::string_view error_code_msg[] = 
                    {
                        [(int)SFETCH_ERROR_NO_ERROR] = "SFETCH_ERROR_NO_ERROR",
                        [(int)SFETCH_ERROR_FILE_NOT_FOUND] = "SFETCH_ERROR_FILE_NOT_FOUND",
                        [(int)SFETCH_ERROR_NO_BUFFER] = "SFETCH_ERROR_NO_BUFFER",
                        [(int)SFETCH_ERROR_BUFFER_TOO_SMALL] = "SFETCH_ERROR_BUFFER_TOO_SMALL",
                        [(int)SFETCH_ERROR_UNEXPECTED_EOF] = "SFETCH_ERROR_UNEXPECTED_EOF",
                        [(int)SFETCH_ERROR_INVALID_HTTP_STATUS] = "SFETCH_ERROR_INVALID_HTTP_STATUS",
                        [(int)SFETCH_ERROR_CANCELLED] = "SFETCH_ERROR_CANCELLED",
                    };
                    std::println(stderr, "Failed to load file, the response error code was: {} -> {}", (int)response->error_code, error_code_msg[(int)response->error_code]);
                }
            },
            .chunk_size = 0,
            .buffer = sfetch_range_t{ .ptr = _m->buffers_per_channel[channel].get(), .size = io_file_reader::channel_buffer_size },
            .user_data = sfetch_range_t{ .ptr = &members_ptr, .size = sizeof(_m.get()) },
        };

        uint32_t request_id = sfetch_send(&request).id;

the buffer size was 10MB (aka io_file_reader::channel_buffer_size == 10MB)

the file was 1.8kb

Just for future reference, does the chunk size mean: 'only load x amount of data from a file'? I interpreted it to be the 'limit' of how many bytes it could read per do work call but that makes no sense now that i write it down haha

floooh commented 2 weeks ago

Yeah I agree that the chunk_size behaviour is confusing. Basically as soon as it is >0 sokol_fetch.h switches into a different mode internally where it wants to load the file in smaller chunks (it's mainly intended for streaming large files in small chunks - for instance video data).

I'll keep the ticket open because I should at least do something about the issue, and if it's just improving the documentation, or communicate a better error.

Regardless, thanks for the ticket and making me aware of the issue!

WillisMedwell commented 2 weeks ago

thanks for the help, it's a great library!