-
Notifications
You must be signed in to change notification settings - Fork 81
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
Build iolist for WebSocket frame data #390
Conversation
Note I am currently getting a spurious test failure which I haven't been able to narrow down yet:
|
I love this! Do you think it would be possible / sensible to keep the extractor logic outside of WebSocket (ie: |
I cannot figure out what is causing the tests to fail. When I add a warning to one of the areas I suspected the test failure completely stops and I am unable to get it to happen unless I remove it, but sometimes it doesn't happen even without this. --- a/lib/bandit/websocket/handler.ex
+++ b/lib/bandit/websocket/handler.ex
@@ -56,6 +56,7 @@ defmodule Bandit.WebSocket.Handler do
end
{extractor, {:error, reason}} ->
+ IO.warn inspect(reason)
{:error, {:deserializing, reason}, %{state | extractor: extractor}}
{extractor, :more} -> I also tried disabling the async runs and just running the one failing test but cannot get anything to behave consistently. Could you have a look at this? |
I guess it shouldn't really have been a surprise that the test that broke depended on certain performance characteristics surrounding sending massive frames to the server. The test is broken in two ways, and both are race conditions:
It tries to ensure 1 happens by adding a defmodule Test do
def listen do
{:ok, listener} = :gen_tcp.listen(5678, [:binary])
{:ok, socket} = :gen_tcp.accept(listener)
:gen_tcp.send(socket, "hello world")
:gen_tcp.shutdown(socket, :write)
Process.sleep(2000)
end
def connect do
{:ok, socket} = :gen_tcp.connect(~c"localhost", 5678, [:binary])
Process.sleep(1000)
{:ok, "hello world"} = :gen_tcp.recv(socket, 1460)
end
end
_p1 = spawn(fn -> Test.listen() end)
Process.sleep(100)
_p2 = spawn(fn -> Test.connect() end)
Process.sleep(2000) This is arguably an OTP bug? Anyway, please make a separate fix for these issues or push to my branch because I can't justify spending any more time on this test issue. |
Agreed that it's pretty racy (a good chunk of the test suite ~unavoidably is). We can skip those tests for now to get this green |
Also just a heads up that I'll be pretty unresponsive until early next week (on vacation). I'll be able to spend some proper time with this issue early next week. |
Merged! Thanks for the PR! |
Fixes #389
This is a significant refactor of the initial proposed solution, but the core idea is the same: when parsing a frame, gather enough bytes to parse the header, and then start receiving the payload data into an iolist until the number of payload bytes is satisfied.