Stiffstream / restinio

Cross-platform, efficient, customizable, and robust asynchronous HTTP(S)/WebSocket server C++ library with the right balance between performance and ease of use
Other
1.15k stars 93 forks source link

Error using sendfile under Windows #21

Closed rcane closed 5 years ago

rcane commented 5 years ago

When using sendfile in a response the file being sent will be closed two times (with the second attempt obviously failing).

The first closing (i.e. a call to ::CloseHandle) is made when the file sending has finished and the writable_item_t wrapping the file is destroyed. This is triggered from connection_t::finish_handling_current_write_ctx.

After this the a send_file_operation_runner_t is destroyed. This class has a member m_file_handle of type asio::windows::random_access_handle. The problem is now that this member also got the file handle when it was constructed. And it wants to close the file in its destruction as well.

I don't really understand the code so I cannot tell which part is supposed to close the file handle. But asio::windows::random_access_handle looks like it is designed to take ownership of the handle. So to fix the problem file_descriptor_holder_t inside send_file_t needs to be released explicitly somewhere to prevent its destructor from closing the handle prematurely.

eao197 commented 5 years ago

Thanks for reporting this! We'll investigate it too.

rcane commented 5 years ago

I found a solution that seems to work.

In the constructor of sendfile_operation_runner_t in sendfile_operation_win.inl, instead of using the file handle inside sendfile_t to construct m_file_handle, I duplicate the handle using DuplicateHandle and pass that to m_file_handle. This allows RESTinio to close the original handle and asio to close the duplicated handle without them interfering each other.

This is how the modified constructor of sendfile_operation_runner_t would look like.

sendfile_operation_runner_t(
    const sendfile_t & sf,
    asio_ns::executor executor,
    asio_ns::ip::tcp::socket & socket,
    after_sendfile_cb_t after_sendfile_cb )
    :   base_type_t{ sf, std::move( executor), socket, std::move( after_sendfile_cb ) }
    ,   m_file_handle{ m_socket.get_executor().context() }
{
    file_descriptor_t duplicate_file_descriptor;
    BOOL ok =
        ::DuplicateHandle(
            ::GetCurrentProcess(),
            m_file_descriptor,
            ::GetCurrentProcess(),
            &duplicate_file_descriptor,
            0,
            FALSE,
            DUPLICATE_SAME_ACCESS);

    if (!ok)
        throw exception_t{"sendfile failed to acquire a file handle"};

    m_file_handle.assign(duplicate_file_descriptor);
}
eao197 commented 5 years ago

Thanks for your proposal, but I can take some time to deal with this issue only after a couple of days :( Sorry for the delay.

rcane commented 5 years ago

No rush. I can work with my local change for now.

eao197 commented 5 years ago

Master branch (as well as default branch on BitBucket) now contains changes which (I hope) fix four issues:

It would be great if you can try them and provide some feedback.

rcane commented 5 years ago

All problems seem to be fixed now.