JuliaWeb / WebSockets.jl

A WebSockets library for Julia
MIT License
157 stars 58 forks source link

improve error on client abrupt close #153

Closed SimonDanisch closed 3 years ago

SimonDanisch commented 5 years ago

Got into a debugging session, where the browser would just close the socket because of julia's JIT lag on first request. The error on the julia side was a bounds error, which is just pretty uninformative. This improves the situation for an error. I now also warmup the server by calling the handler with a websocket request:

headers = [
        "Host" => "127.0.0.1",
        "User-Agent" => "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0",
        "Accept" => "*/*",
        "Accept-Encoding" => "gzip, deflate, br",
        "Accept-Language" => "de,en-US;q=0.7,en;q=0.3",
        "Cache-Control" => "no-cache",
        "Connection" => "keep-alive, Upgrade",
        "Dnt" => "1",
        "Origin" => "https://localhost",
        "Pragma" => "no-cache",
        "Sec-Websocket-Extensions" => "permessage-deflate",
        "Sec-Websocket-Key" => "BL3d8I8KC5faPjubRM0riA==",
        "Sec-Websocket-Version" => "13",
        "Upgrade" => "websocket",
]
msg = HTTP.Request(
        "GET",
        "/",
        headers,
        UInt8[],
        parent = nothing,
        version = v"1.1.0"
    )
stream = Stream(msg, IOBuffer())

stream_handler(application, stream)

I'm doing this in my app atm, but I think ultimately, the warm up should go into WebSockets/HTTP, since it's nice for testing, and also pretty fundamental since the jit lag is bad enough to make things error.

hustf commented 5 years ago

Build success! I had a look at the first failed run, and just really liked the informative error message. Will have a closer look at first oportunity.

I dont know if this is relevant in your case, but sending a ping through the entire chain may help initial response, say the first time a user clicks somewhere.

SimonDanisch commented 5 years ago

Btw, I'm not sure if I should throw a websocket error, since it then tries to do a closing response, even though an EOF error should indicate that the connection is already closed...

SimonDanisch commented 3 years ago

I have close to 0 memories of this PR :D but I'm somewhat convinced, that reading the bytes one by one had a reason. I think it was because I run into bound errors, since sometimes it seems there are situation where one can read only one byte - and with this change, it will then be an IO error, which gets caught correctly

hustf commented 3 years ago

Thank you for the quick response! That's a reason, then. We always knew it was a hack, and the multiple new versions since I tried that may have tightened up Julia's bound checking anyway. I was extremely concerned about response time at that time.

Before merging: I believe you may have switched to HTTP.WebSockets for you app - do you have any new insights regarding the end-of-message behaviour? Would you add any changes, or should we merge to master?

SimonDanisch commented 3 years ago

🤷 I have no idea :D I switched half over to HTTP... And am trying to switch over JSServe to HTTP as well, but that runs into a few other edge cases

hustf commented 3 years ago

Ok. We'll just merge then. The variable name that I didn't like will be your own contribution to WebSockets' ugliness.

Regarding warm-up. I suppose that's for another PR but since I have you online: I tried (in a private project) to implement something like the example in 'Module initialization and precompilation'. But it didn't work out for me then, and I'm way out of my depth regarding how compilation and warm-up works. If you actually were having clients getting too impatient, it means it's a real issue and our tests aren't strict enough even now. I believe we mentioned the 'ping' tricks in the readme, but doing this in init() would be great if it works.