-
Notifications
You must be signed in to change notification settings - Fork 57
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
improve error on client abrupt close #153
Conversation
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. |
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... |
fin = (a & 0b1000_0000) >>> 7 # If fin, then is final fragment | ||
rsv1 = a & 0b0100_0000 # If not 0, fail. | ||
rsv2 = a & 0b0010_0000 # If not 0, fail. | ||
rsv3 = a & 0b0001_0000 # If not 0, fail. | ||
opcode = a & 0b0000_1111 # If not known code, fail. | ||
|
||
b = ab[2] | ||
b = read(ws.socket, UInt8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is quite finicky, I remember some quite unexpected behaviour when trying to change it earlier. It's actually part of the motivation for having the very large test suite now. We want to trigger those weird edge cases by testing very thoroughly.
Now that we have extensive tests, and they don't fail, we could make this change I suppose. Unfortunately the benchmark tests were never really completed so would the two read operations be faster? Is there any other motivation than beautification?
errtyp = typeof(err) | ||
if errtyp <: InterruptException | ||
msg = " while read(ws|$(ws.server ? "server" : "client") received InterruptException." | ||
server_str = ws.server ? "server" : "client" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server_str -> type_str
@@ -360,18 +360,18 @@ function handle_control_frame(ws::WebSocket, wsf::WebSocketFragment) | |||
ws.state = CLOSED | |||
try | |||
locked_write(ws.socket, true, OPCODE_CLOSE, !ws.server, UInt8[]) | |||
catch | |||
catch e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually assigning the error could be expensive, and it's not actually used?
Or is the compiler smart enough to avoid doing unnecessary work here?
throw(WebSocketClosedError(" while read(ws|$(ws.server ? "server" : "client")) $(string(err))")) | ||
throw(WebSocketClosedError("while read(ws|$(server_str)) $(err.message) - Performed closing handshake.")) | ||
elseif err isa Base.IOError || err isa Base.EOFError | ||
throw(WebSocketClosedError("while read(ws|$(server_str)) $(string(err))")) | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making more distictions like this must be a good thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In doubt about whether reading the first two bytes in two read operations is an improvement (line 404). As it is (reading two bytes at once) is a bit of a hack, but we tried this before and went back. Don't know if the previous try is traceable or not, but the syntax for reading has most likely changed anyway.
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. |
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? |
🤷 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 |
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. |
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:
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.