-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Fix infinite stream and its missing incumbent script environment when newing a new stream #26810
Conversation
Heads up! This PR modifies the following files:
|
cd668f2
to
4835422
Compare
hmmm.... I tried several ways for an unit test but didn't figure out a workable version :/ a simplest test is like this; I confirmed it will get into the IPC router but we will even get it pass without patches in this PR... 🤔 promise_test((t) => {
var blob = new Blob(["This is my blob content"], { type: "text/plain" });
var formData = new FormData();
formData.append("blob", blob, "blob.txt");
return fetch("/fetch/api/resources/echo-content.py", {
method: "POST",
body: formData,
});
}, "Fetch with POST blob"); |
@bors-servo try=wpt
|
Fix infinite stream and its missing incumbent script environment when newing a new stream --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #26807 - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___
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.
Thanks for the fixes! Just a few comments...
I'm not sure about the tests, actually I was surprised this wasn't caught before. Maybe you need a bit of a delay like would happen in a real network, you can try using https://web-platform-tests.org/writing-tests/server-pipes.html#trickle
💔 Test failed - status-taskcluster |
hmm... interesting, even I do |
ah!! cool! I think |
hmm, this is not true; |
4835422
to
99ab109
Compare
ok, I can reproduce these |
I know what the problem is, so let me know if you want to try to find it yourself first 😄 hint: look at what is done to the |
Actually I'm not so sure I know what the problem is, but yeah the You might also want to check the interplay between the below and what's happening in servo/components/net_traits/request.rs Line 173 in 99ab109
I'm just wondering if it would be possible for the @jdm I couldn't find the details in the semantics of
Can you be certain that the other side will first receive the message, and then notice the channel being closed? Or is it possible for the channel to close first? That's one thing I can imagine could cause a problem if the route handler is removed too early. Because in theory if we first receive the message, then a new route is setup before the old one being removed, but if the channel closes first, it could break the re-direct. @CYBAI we could test this by ensuring the |
Yeah reading on it, it seems if you close a file descriptor, you loose all data in transit, so it reads a bit like dropping the chan might close the receiver, and remove the route, even though there is still a message in transit. @CYBAI you could try to add a servo/components/net_traits/request.rs Line 176 in 99ab109
do a I think it's the servo/components/net/http_loader.rs Line 449 in 0645d8c
which then drops it's IpcSender<BodyChunkRequest> , and then the RequestBody still has a sender, however it is then dropped on the next take_stream , and then the route in body.rs closes before the new route for the re-direct in setup in response to receiving the BodyChunkRequest::Extract message.
So previously we would not actually call the Maybe, it's worth a try... |
@gterzian thanks for the tip! I will try it later! |
@gterzian I tried to debug with removing the I haven't tried the Then, I just realized, in the step 5.1.3 of request transmit body, it says The servo/components/script/dom/readablestream.rs Lines 503 to 505 in 0645d8c
I'm wondering, if we can know the stream is done or not by that property, maybe we don't need to add a flag like But, here comes my question: I found I don't really understand why if we have something in memory, we will just send it and return 🤔 If we can check if done or not by the servo/components/script/body.rs Lines 165 to 169 in 0645d8c
|
Yes, thanks for looking into these matters. So in the case where this problem happens, we have all the bytes in memory, and we don't actually go through the stream and SpiderMonkey. So we're not following the spec to the letter in that case, and I think that's ok(there will be many more cases like this, actually we should find a better way to do this, see for example #26709, #26759, and #26686). So we need the
Then I think using this |
Indeed! When there's no need to do chunks, it makes sense to just return the in-memory bytes! Okay! I will give |
@gterzian I found I don' fully understand what you mean here. Do you mean, we shouldn't drop the Does that mean we should drop it in this router? servo/components/script/body.rs Line 338 in aaa6cea
|
It's just what when we do So then I think this is causing the current problem, because there are two places that own a
The sender in So I think previously, during a re-direct, when So now we're calling Or at least I think that is what is happening. And we can solve it by not dropping those sender upon a redirect in |
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.
Ok great, few last comments left.
1d0c068
to
e928b38
Compare
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.
Ok, thanks for the latest fixes. There is still one point that we should address I think.
e928b38
to
1d92be6
Compare
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.
Looks good, there's just one comment that can be made more effective I think.
1d92be6
to
cf7650f
Compare
As discussed in servo#26807 (comment), we'd like to add a new flag, `in_memory_done`, to `TransmitBodyConnectHandler` so that we can correctly finish and drop the sender correctly. When we send the bytes, we will mark the body as done and we can recognize it's already done in next tick so that we can send a Done request to finish the sender. Also, when there comes a redirect request, it will go to `re-extract` route, we can set the `done` flag to `false` which means we won't stop the IPC routers yet. Then, if the re-extract sent its bytes, we will be marked as done again so that we can finish with stopping the IPC routes when Chunk request comes.
cf7650f
to
63aab1b
Compare
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.
Great work!
@bors-servo r+
@bors-servo r+ |
📌 Commit 63aab1b has been approved by |
Ha, this reminds me homu doesn't support commands from review comment yet 😂 |
Fix infinite stream and its missing incumbent script environment when newing a new stream --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #26807 - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___
💔 Test failed - status-taskcluster |
@bors-servo retry #26895 |
☀️ Test successful - status-taskcluster |
./mach build -d
does not report any errors./mach test-tidy
does not report any errors