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

Replace ws-rs with async-tungstenite. #27164

Merged
merged 1 commit into from Jul 8, 2020
Merged

Conversation

jdm
Copy link
Member

@jdm jdm commented Jul 3, 2020

This change moves us from ws-rs (which doesn't see a lot of maintainer activity) and its custom async implementation to tungstenite and the tokio ecosystem. This is particularly important because of #27043, which breaks SSL websockets on Windows.

Depends on sdroege/async-tungstenite#40.


@highfive
Copy link

highfive commented Jul 3, 2020

Heads up! This PR modifies the following files:

  • @KiChjang: components/net/Cargo.toml, components/net/resource_thread.rs, components/net/websocket_loader.rs

@@ -0,0 +1,3 @@
[third-party-cookie-accepted.https.html]
Copy link
Member Author

@jdm jdm Jul 3, 2020

Choose a reason for hiding this comment

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

This new test failure shows up because tungstenite correctly passes through multiple Set-Cookie headers in the WS response, whereas ws-rs appears to silently drop all but the first.

@jdm
Copy link
Member Author

jdm commented Jul 3, 2020

r? @nox
Do you want to take a look at this?

@highfive highfive assigned nox and unassigned SimonSapin Jul 3, 2020
@nox
Copy link
Member

nox commented Jul 3, 2020

This looks cool! Will review on Monday!

@jdm
Copy link
Member Author

jdm commented Jul 3, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

Trying commit bed15c7 with merge 659d1b4...

bors-servo added a commit that referenced this issue Jul 3, 2020
Replace ws-rs with async-tungstenite.

This change moves us from ws-rs (which doesn't see a lot of maintainer activity) and its custom async implementation to tungstenite and the tokio ecosystem. This is particularly important because of #27043, which breaks SSL websockets on Windows.

Depends on sdroege/async-tungstenite#40.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27043
- [x] There are tests for these changes
@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Jul 3, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

Trying commit 7376630 with merge 1a8e80f...

bors-servo added a commit that referenced this issue Jul 3, 2020
Replace ws-rs with async-tungstenite.

This change moves us from ws-rs (which doesn't see a lot of maintainer activity) and its custom async implementation to tungstenite and the tokio ecosystem. This is particularly important because of #27043, which breaks SSL websockets on Windows.

Depends on sdroege/async-tungstenite#40.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27043
- [x] There are tests for these changes
@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Jul 3, 2020

Ok, the test failures appear to be related to the host replacement that happens for WPT which confuses openssl. I need to figure out what we do differently in HTTPS code that allows it to work.

@jdm
Copy link
Member Author

jdm commented Jul 3, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

Trying commit 806133d with merge e05cc5c...

bors-servo added a commit that referenced this issue Jul 3, 2020
Replace ws-rs with async-tungstenite.

This change moves us from ws-rs (which doesn't see a lot of maintainer activity) and its custom async implementation to tungstenite and the tokio ecosystem. This is particularly important because of #27043, which breaks SSL websockets on Windows.

Depends on sdroege/async-tungstenite#40.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27043
- [x] There are tests for these changes
@bors-servo
Copy link
Contributor

bors-servo commented Jul 3, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Jul 8, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

Testing commit 76198e4 with merge f5666f7...

bors-servo added a commit that referenced this issue Jul 8, 2020
Replace ws-rs with async-tungstenite.

This change moves us from ws-rs (which doesn't see a lot of maintainer activity) and its custom async implementation to tungstenite and the tokio ecosystem. This is particularly important because of #27043, which breaks SSL websockets on Windows.

Depends on sdroege/async-tungstenite#40.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27043
- [x] There are tests for these changes
@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Jul 8, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

Testing commit 76198e4 with merge d6e29a8...

bors-servo added a commit that referenced this issue Jul 8, 2020
Replace ws-rs with async-tungstenite.

This change moves us from ws-rs (which doesn't see a lot of maintainer activity) and its custom async implementation to tungstenite and the tokio ecosystem. This is particularly important because of #27043, which breaks SSL websockets on Windows.

Depends on sdroege/async-tungstenite#40.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #27043
- [x] There are tests for these changes
@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member Author

jdm commented Jul 8, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

Testing commit 76198e4 with merge ed82722...

@bors-servo
Copy link
Contributor

bors-servo commented Jul 8, 2020

☀️ Test successful - status-taskcluster
Approved by: nox
Pushing ed82722 to master...

@bors-servo bors-servo merged commit ed82722 into servo:master Jul 8, 2020
2 checks passed
bors-servo added a commit that referenced this issue Jul 16, 2020
Remove explicit Host header from websocket connection.

This was a regression from #27164, and fixing it allows hubs.mozilla.com to work again.
bors-servo added a commit that referenced this issue Jul 16, 2020
Remove explicit Host header from websocket connection.

This was a regression from #27164, and fixing it allows hubs.mozilla.com to work again.
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.

5 participants