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

check if close code exists before reading it #192

Merged
merged 2 commits into from
Apr 21, 2018

Conversation

fuchsnj
Copy link
Contributor

@fuchsnj fuchsnj commented Apr 21, 2018

closes #191

@codecov
Copy link

codecov bot commented Apr 21, 2018

Codecov Report

Merging #192 into master will decrease coverage by 1.73%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
- Coverage   78.32%   76.58%   -1.74%     
==========================================
  Files          90       90              
  Lines       11292    11301       +9     
==========================================
- Hits         8844     8655     -189     
- Misses       2448     2646     +198
Impacted Files Coverage Δ
tests/test_ws.rs 90.69% <100%> (+0.69%) ⬆️
src/ws/mod.rs 84.84% <100%> (+0.28%) ⬆️
src/server/worker.rs 8% <0%> (-64%) ⬇️
src/server/srv.rs 30.06% <0%> (-29.73%) ⬇️
tests/test_server.rs 86.57% <0%> (-9.6%) ⬇️
src/server/mod.rs 16.66% <0%> (-8.34%) ⬇️
src/server/h1writer.rs 85.5% <0%> (-5.08%) ⬇️
src/server/settings.rs 83.92% <0%> (-3.58%) ⬇️
src/ws/frame.rs 77.11% <0%> (-3.53%) ⬇️
src/client/connector.rs 52.5% <0%> (-2.23%) ⬇️
... and 2 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 9880a95...dc9a24a. Read the comment docs.

@fafhrd91
Copy link
Member

Do you want to use Option?

@fuchsnj
Copy link
Contributor Author

fuchsnj commented Apr 21, 2018

Yes. I wasn't sure of the policy for breaking changes on this project. Since you are ok with #193 I'll go ahead and add that here

@fafhrd91
Copy link
Member

Should we use 1005 for 0.5 and then make it option in 0.6?

@fuchsnj
Copy link
Contributor Author

fuchsnj commented Apr 21, 2018

Sure, that will fix the panic in 0.5

I'll open a separate PR for #193 then with the breaking changes for 0.6. Feel free to merge this one now if it looks good to you

@fafhrd91
Copy link
Member

thanks!

@fafhrd91 fafhrd91 merged commit 805dbea into actix:master Apr 21, 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.

panic when Websocket is closed with no error code
2 participants