_openstream() may cause data loss

[zhanglix]

The HTTP.startread(http) call will call readheaders which relies on HTTP.IOExtras.unread! to push excess data back to the input stream.

And only using io = HTTP.ConnectionPool.getrawstream(http) in WebSocket will cause any excess data on http stream lost.

In case that the server send one short message immediately on connected event, and the first time to call _openstream, the odds of losing data is nearly 100%.

function _openstream(f::Function, http::HTTP.Streams.Stream, key::String)

    HTTP.startread(http) # This method may read more bytes then need, and unread!(http) will be called.

    status = http.message.status
    if status != 101
        return
    end

    check_upgrade(http)

    if HTTP.header(http, "Sec-WebSocket-Accept") != generate_websocket_key(key)
        throw(WebSocketError(0, "Invalid Sec-WebSocket-Accept\n" *
                                "$(http.message)"))
    end

    io = HTTP.ConnectionPool.getrawstream(http) # the bytes from unread!() will can not be read anymore
    ws = WebSocket(io,false)
    try
        f(ws)
    finally
        close(ws)
    end

end

---

A simple testing case

A echo server which will send a message to client first.

#!/usr/bin/env python

import asyncio
import websockets

async def echo(websocket, path):
    msg = "This is a dummy echo server."
    peer = str(websocket.remote_address)
    try:
        while True:
            await websocket.send(msg)
            print("[{}]> {}".format(peer,msg))
            msg = await websocket.recv()
            print("[{}]> {}".format(peer,msg))
    except Exception as e:
        print("[{}]> closed.".format(peer)) 

start_server = websockets.serve(echo, 'localhost', 8765)

asyncio.get_event_loop().run_until_complete(start_server)
asyncio.get_event_loop().run_forever()

The following julia code will hang on the read(ws) call with high odds.

using WebSockets
WebSockets.open("ws://localhost:8765") do ws::WebSocket
        read(ws)
end

[hustf]

Thanks for the issue. First step is confirmation. With the Python server above, read never exited as it should on the first three tries.

Modifying examples/minimal server.jl to first send a message worked as intended on the first try, i.e. the function exits with the data sent by minimal server.jl. The client code below was run in a separate REPL.

Without closing Julia and simply rerunning the client code, 'read' never receives the first message fully, i.e. it never exits.

I am on travel. Nice if you can look further into this.

Client repl:

using WebSockets
using Sockets
WebSockets.open("ws://$(getipaddr()):8080") do ws::WebSocket
                     data, bool = readguarded(ws)
                     println("Success: ", bool)
                     println("Message read at once: ", String(data))
                end

Server repl: Modifed example code:

function coroutine(ws)
    while isopen(ws)
        writeguarded(ws, "Welcome to this server!")
        data, = readguarded(ws)
        s = String(data)
        if s == ""
            writeguarded(ws, "Goodbye!")
            break
        end
        println("Received: ", s)
        writeguarded(ws, "Hello! Send empty message to exit, or just leave.")
    end
end

(edit: removed actual IP address, multi line in client test code).

[hustf]

Checking if this might be acceptable behaviour given that it is kind of unnatural for a web server to send the first socket message: it is not acceptable. From standard: " This completes the server's handshake. If the server finishes these steps without aborting the WebSocket handshake, the server considers the WebSocket connection to be established and that the WebSocket connection is in the OPEN state. At this point, the server may begin sending (and receiving) data."

[zhanglix]

This happens to be exactly my use case. The WebSocket Server act as a configuration service. Any client connecting to it will receive the current config immediately. And the client will receive notification on configuration changes. A client will never send message to the server unless it need to make some changes to the config.

[hustf]

I have the same use case, where the server acts as a relay station between calculation and browser. This works on julia 0.6 and with HttpServer. We need to fix it if we can.

[zhanglix]

one simple (but need lots of coding) is store HTTP.Stream.Stream in WebScoket{T} then add corresponding read write methods for it.

[hustf]

Thanks again! If nobody else chimes in, we'll merge your changes after some cosmetic changes. This may be temporary code, in case there is a good way to fix the underlying issue in HTTP.jl. That package has many more concerns, so changing the effect of some functions will take some condsideration.

Heads-up for this important issue to websocket experts @erikedin (DandelionWebSockets.jl), @samoconnor (HTTP.jl).

[zhanglix]

Wait some more hours. I need to refactor the tests and open a new pull request later.

[zhanglix]

Thanks again! If nobody else chimes in, we'll merge your changes after some cosmetic changes. This may be temporary code, in case there is a good way to fix the underlying issue in HTTP.jl. That package has many more concerns, so changing the effect of some functions will take some condsideration.

Heads-up for this important issue to websocket experts @erikedin (DandelionWebSockets.jl), @samoconnor (HTTP.jl).

I agree, if HTTP.jl could be more friendly to this use case, all this hacking is not necessary.

[hustf]

Just tagged release v1.0.1 after also picking some very low hanging fruit. There is plenty left since the Julia 0.7 / 1.0 release in case you would like to contribute!

[samoconnor]

See https://github.com/JuliaWeb/HTTP.jl/pull/305

[hustf]

That's great! We'll keep this patch until benchmarks are up and running again. They tend to provoke the borderline errors.