Skip to content
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

Add ReadyState, fix is_websocket_upgrade and add tests #194

Closed
wants to merge 1 commit into from

Conversation

EricForgy
Copy link
Contributor

Sorry! My bad 😅

I copy / pasted your suggestion (including a typo) without checking 😅

@EricForgy
Copy link
Contributor Author

I'll try to add some tests...

@codecov-io
Copy link

codecov-io commented Feb 11, 2018

Codecov Report

Merging #194 into master will decrease coverage by 46.63%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #194       +/-   ##
===========================================
- Coverage   72.93%   26.29%   -46.64%     
===========================================
  Files          33       33               
  Lines        1740     1734        -6     
===========================================
- Hits         1269      456      -813     
- Misses        471     1278      +807
Impacted Files Coverage Δ
src/WebSockets.jl 75.42% <94.44%> (+10%) ⬆️
src/Strings.jl 0% <0%> (-100%) ⬇️
src/BasicAuthRequest.jl 0% <0%> (-100%) ⬇️
src/Handlers.jl 0% <0%> (-95.13%) ⬇️
src/sniff.jl 0% <0%> (-92.63%) ⬇️
src/cookies.jl 0% <0%> (-91.47%) ⬇️
src/CookieRequest.jl 0% <0%> (-90.91%) ⬇️
src/URIs.jl 7.27% <0%> (-83.64%) ⬇️
src/MessageRequest.jl 23.8% <0%> (-76.2%) ⬇️
src/TimeoutRequest.jl 0% <0%> (-70%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4aa53a4...6f5d7be. Read the comment docs.

@EricForgy
Copy link
Contributor Author

I can't get the test to pass. This might be beyond my skill level 😭

The first two tests pass on the local WS server, but when I try to write from a WS to IOBuffer, it hangs. Even close hangs. I can't figure it out, so I rebased my PR so maybe you can have some suggestions.

@EricForgy
Copy link
Contributor Author

I don't see any place in the code where a socket is ever actually closed...

@EricForgy
Copy link
Contributor Author

I'm playing with the idea of changing:

txclosed::Bool -> txstate::ReadyState

and

rxclosed::Bool -> rxstate::ReadyState

where

@enum ReadyState CONNECTED=0x1 CLOSING=0x2 CLOSED=0x3

This is from the old WebSockets.jl. Since I (we?) are trying to support both client and server, we need some way to avoid bouncing close messages back and forth. If one side is in a CLOSING state (meaning it has sent a close request) and receives a close message, it is safe to terminate the connection.

Any thoughts?

@EricForgy
Copy link
Contributor Author

I made some progress in the latest commit. 2 of 3 tests pass, but I'm still having some difficulty getting `

write(to::IO, from::HTTP.WebSockets.WebSocket)

to work properly with the local WS server.

I'm also getting

ArgumentError("stream is closed or unusable")

and haven't been able to track it down yet.

@EricForgy EricForgy changed the title Fix typo in is_websocket_upgrade Add ReadyState, fix is_websocket_upgrade and add tests Feb 12, 2018
@EricForgy
Copy link
Contributor Author

#197 would make this moot 🙏

is_websocket_upgrade(r::HTTP.Message) =
(r isa HTTP.Request && r.method == "GET" || r.status == 101) &&
HTTP.hasheader(r, "Connection", "upgrade") &&
HTTP.hasheader(r, "Upgrade", "webscoket")
HTTP.hasheader(r, "Upgrade", "websocket")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops!
Thankyou :)


close(io)
p = 8000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added these tests 2cea2f7

@EricForgy
Copy link
Contributor Author

👍

@EricForgy EricForgy closed this Feb 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants