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

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

Open
WillisMedwell opened this issue Jun 11, 2024 · 7 comments
Open

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

WillisMedwell opened this issue Jun 11, 2024 · 7 comments

Comments

@WillisMedwell
Copy link

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
Copy link
Owner

floooh commented Jun 12, 2024

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

I suspect the problematic code is this:

sokol/sokol_fetch.h

Lines 2225 to 2239 in c54523c

if ((req.status == 206) || ((req.status == 200) && !need_range_request)) {
const u8_array = new Uint8Array(\x2F\x2A\x2A @type {!ArrayBuffer} \x2A\x2F (req.response));
const content_fetched_size = u8_array.length;
if (content_fetched_size <= buf_size) {
HEAPU8.set(u8_array, buf_ptr);
__sfetch_emsc_get_response(slot_id, bytes_to_read, content_fetched_size);
}
else {
__sfetch_emsc_failed_buffer_too_small(slot_id);
}
}
else {
__sfetch_emsc_failed_http_status(slot_id, req.status);
}
}

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

sokol/sokol_fetch.h

Lines 2200 to 2207 in c54523c

if (req.readyState == XMLHttpRequest.DONE) {
if (req.status == 200) {
const content_length = req.getResponseHeader('Content-Length');
__sfetch_emsc_head_response(slot_id, content_length);
}
else {
__sfetch_emsc_failed_http_status(slot_id, req.status);
}

...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
Copy link
Author

  • Callstack where fail is set:
    image

  • Chunk Size
    Tried both at .chunk_size = 1024*1024; and chunk_size = {};and still had the same issue.

  • Network
    The network info seems to be all okay to me as well. The data received is as expected too.
    image
    image
    image
    this is the test data i expected (dw about it's just for testing).

  • Setup values
    just in case...

       constexpr static auto sokol_setup_settings = sfetch_desc_t {
            .max_requests = 512,
            .num_channels = 4,
            .num_lanes = 1,
        };

        sfetch_setup(&sokol_setup_settings);

@floooh
Copy link
Owner

floooh commented Jun 12, 2024

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

sokol/sokol_fetch.h

Lines 2213 to 2242 in c54523c

/* if bytes_to_read != 0, a range-request will be sent, otherwise a normal request */
EM_JS(void, sfetch_js_send_get_request, (uint32_t slot_id, const char* path_cstr, uint32_t offset, uint32_t bytes_to_read, void* buf_ptr, uint32_t buf_size), {
const path_str = UTF8ToString(path_cstr);
const req = new XMLHttpRequest();
req.open('GET', path_str);
req.responseType = 'arraybuffer';
const need_range_request = (bytes_to_read > 0);
if (need_range_request) {
req.setRequestHeader('Range', 'bytes='+offset+'-'+(offset+bytes_to_read-1));
}
req.onreadystatechange = function() {
if (req.readyState == XMLHttpRequest.DONE) {
if ((req.status == 206) || ((req.status == 200) && !need_range_request)) {
const u8_array = new Uint8Array(\x2F\x2A\x2A @type {!ArrayBuffer} \x2A\x2F (req.response));
const content_fetched_size = u8_array.length;
if (content_fetched_size <= buf_size) {
HEAPU8.set(u8_array, buf_ptr);
__sfetch_emsc_get_response(slot_id, bytes_to_read, content_fetched_size);
}
else {
__sfetch_emsc_failed_buffer_too_small(slot_id);
}
}
else {
__sfetch_emsc_failed_http_status(slot_id, req.status);
}
}
};
req.send();
});

...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:

  • Are you explicitely setting the chunk_size in the sfetch_send() call to a value > 0? Wait, you do...
  • ...and are you actually intending to only load a smaller part of the whole file, or do you want to load the entire file? (in the latter case, keep the chunk_size set to zero)
  • I see that you are using the Python HTTP server, as far as I remember this didn't support ranged-requests and instead always returns the whole file and 200 status code, for testing ranged-requested I had to switch to the node.js http-server command instead.

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
Copy link
Owner

floooh commented Jun 12, 2024

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
Copy link
Author

WillisMedwell commented Jun 12, 2024

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
Copy link
Owner

floooh commented Jun 12, 2024

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
Copy link
Author

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

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

No branches or pull requests

2 participants