Possible bug with sending binary data

[Tectu]

I'm in the process of updating one of my application that consume malloy to the latest main branch (the application in question still uses malloy from before @0x00002a 's heavy rework).

Before the rework, WebSocket connections were hardcoded to use binary mode. This was dropped 72a5678993e30cc843babf278d6a7d37f6bd56ab introduced a configuration option for this instead. Currently I experience problems that the data that the client sends comes in with some partial garbling at the beginning on the server side. Both client & server use conn->set_binary(true) immediately as the first statement in the respective handlers for client::controller::ws_connect() and server::router::add_websocket().

I'm currently not sure whether this is a bug in malloy or whether I am overlooking something obvious. In my particular case the client sends an XML document as a string over the websocket connection:

Client sends:

<configuration type="foo">
    <dummy/>
</configuration>

Server recieves:

 â┘ºj☻  αl┘ºj☻  iguration type="foo">
    <dummy/>
</configuration>

Relevant client code:

void
handler::run_document_once(
    std::string document_contents,
    std::function<void(const malloy::error_code, const std::string& msg)>&& handler
)
{
    m_malloy_controller->ws_connect(
        m_host,
        m_port,
        "/process/run/once",
        [
            doc = std::move(document_contents),
            handler = std::move(handler)
        ](const malloy::error_code ec, auto conn) {
            conn->set_binary(true);

            if (ec) {
                std::cerr << "ERROR 1: " << ec.message() << std::endl;
                // ToDo
                return;
            }

            conn->send(malloy::buffer(doc), [conn, handler = std::move(handler)](const malloy::error_code& ec, const std::size_t& size){
                if (ec) {
                    std::cerr << "ERROR 2: " << ec.message() << std::endl;
                    // ToDo
                    return;
                }

                auto buffer = std::make_shared<boost::beast::flat_buffer>();
                conn->read(*buffer, [buffer, handler = std::move(handler)](auto ec, auto) {
                    std::cout << "foo1"  << std::endl;
                    handler(ec, malloy::buffers_to_string(malloy::buffer(buffer->cdata(), buffer->size())));
                });
            });
        }
    );
}

Relevant server code:

router->add_websocket("/process/run/once", [this](const auto& req, auto conn)
{
    conn->set_binary(true);

    conn->accept(req, [conn, this](){
        spdlog::info("requested: document run once");

        auto buffer = std::make_shared<boost::beast::flat_buffer>();
        conn->read(*buffer, [buffer, conn, this](auto ec, auto) {
            if (ec) {
                spdlog::error("error: {}", ec.message());
                return;
            }

            // Extract payload
            const std::string payload = malloy::buffers_to_string(malloy::buffer(buffer->cdata(), buffer->size()));
            spdlog::warn("payload: \n{}\n", payload);

            // ...

            });
        });
    });
});

@0x00002a am I missing something obvious? Are you by any chance using the new, restructured websocket system in any of your applications?

Note: I am aware that in this case I could send the data as text instead. However, eventually protobuf messages are sent over that connection instead. The parsing of the protobuf message on the server side failed so I started investigating and dumb it down to the part where I simply set the XML as a string rather than encapsulating it in a protobuf message.

[0x00002a]

This part here:

conn->send(malloy::buffer(doc), [conn, handler = std::move(handler)]...

Is the issue. See this line: https://github.com/Tectu/malloy/blob/8e1c364a896b6895bfc6a913fa5423bd5d012707/lib/malloy/websocket/connection.hpp#L198

doc is decontructed and the memory pointed to it (which malloy::buffer wraps but does not own see: asio docs) is deleted almost immediately.

You can wrap it in shared_ptr as a quick and dirty way to keep it around (which is what accept_and_send does iirc). The behaviour is a bit of a gotcha tbh, I'm open to ideas of how to make it less of a sharp corner :p.

[Tectu]

Something about shooting yourself in the foot...? :p The embarrassing thing is that I am/was aware of that and even read that notice when investigating.

This looks like a tricky case to take the edge off. I can currently not think of anything rather than a wrapper which creates the shared_ptr automatically. But then again, the details are so usage specific that it will most likely not be very beneficial of providing an out-of-the-box solution with malloy. I think implementing application specific handlers similar to how the ws_handlers.cpp in /examples does it is the way to go - but that is application side, not malloy side.