Bug 16071 - Curl backend handles EINTR incorrectly
Summary: Curl backend handles EINTR incorrectly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Curl
Depends on:
Blocks:
 
Reported: 2007-11-20 08:09 PST by Rodney Dawes
Modified: 2007-12-18 14:53 PST (History)
1 user (show)

See Also:


Attachments
Patch to handle EINTR correctly in curl backend (1.50 KB, patch)
2007-11-20 08:10 PST, Rodney Dawes
alp: review-
Details | Formatted Diff | Diff
Defer timers during select() (1.84 KB, patch)
2007-11-21 08:47 PST, Alp Toker
no flags Details | Formatted Diff | Diff
Patch to deal with EINTR (1.62 KB, patch)
2007-12-07 11:36 PST, Rodney Dawes
darin: review+
Details | Formatted Diff | Diff
Updated patch with ChangeLog (2.13 KB, patch)
2007-12-18 11:27 PST, Rodney Dawes
alp: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rodney Dawes 2007-11-20 08:09:26 PST
The curl backend is handling EINTR incorrectly by just stopping the stream. It instead needs to try again when EINTR occurs in select(). We also need to use perror() to print the error message when debug is enabled, so that the message is useful, and not plainly "select() return -1". Failing to try again on EINTR means that content is not downloadable as it should be. This occurs quite often on YouTube and similar sites which stream data to a plug-in. By trying again on EINTR we can watch the full videos and have the site behave as expected. With this patch, the CPU usage and network usage patterns match those of firefox when watching the same video on YouTube, where this problem occurs.
Comment 1 Rodney Dawes 2007-11-20 08:10:28 PST
Created attachment 17418 [details]
Patch to handle EINTR correctly in curl backend
Comment 2 Alp Toker 2007-11-20 10:33:26 PST
Comment on attachment 17418 [details]
Patch to handle EINTR correctly in curl backend

Good catch. The recursion definitely needs to go. See my proposed changes
Comment 3 Alp Toker 2007-11-20 10:33:29 PST
Comment on attachment 17418 [details]
Patch to handle EINTR correctly in curl backend

Good catch. The recursion definitely needs to go. See my proposed changes
Comment 4 Alp Toker 2007-11-20 13:04:41 PST
I looked into this a bit more and now have a feeling this is just a symptom of bad curl API use. I'd suspect that it's curl's job to check for situations like this, not ours. Can you find a reproducible non-plugin test case for this?
Comment 5 Rodney Dawes 2007-11-20 14:46:10 PST
No. I can't seem to reproduce this problem exactly. When I try to load a movie file directly, it just gets dumped to the view, and I think ends up spinlocking somewhere in the process.

Is someone rewriting the network backend and has that code in a state that can replace the current curl code?
Comment 6 Alp Toker 2007-11-20 16:57:48 PST
Can you try with no modifications except wrapping the select() with this guard?

    setDeferringTimers(true);
    rc = ::select(maxfd + 1, &fdread, &fdwrite, &fdexcep, &timeout);
    setDeferringTimers(false);

Tell me how it goes.
Comment 7 Rodney Dawes 2007-11-21 07:19:16 PST
(In reply to comment #6)
> Can you try with no modifications except wrapping the select() with this guard?
> 
>     setDeferringTimers(true);
>     rc = ::select(maxfd + 1, &fdread, &fdwrite, &fdexcep, &timeout);
>     setDeferringTimers(false);
> 
> Tell me how it goes.

This seems to also work sufficiently.
Comment 8 Alp Toker 2007-11-21 08:47:46 PST
Created attachment 17431 [details]
Defer timers during select()
Comment 9 Mark Rowe (bdash) 2007-11-21 18:58:05 PST
Comment on attachment 17431 [details]
Defer timers during select()

r=me, though I think the while should be written as while (condition) { } if you're going to squish it all on one line.  It's too easy to overlook the semicolon, and some versions of GCC like to emit warnings for code like this.
Comment 10 Alp Toker 2007-11-21 19:28:27 PST
Landed in r27948.
Comment 11 Rodney Dawes 2007-12-05 13:34:48 PST
Hrmm. This worked when I tested it, but still fails now in SVN.
Comment 12 Darin Adler 2007-12-05 23:01:26 PST
Comment on attachment 17431 [details]
Defer timers during select()

Clearing the review flag since this patch was landed but the bug must remain open.
Comment 13 Rodney Dawes 2007-12-07 11:36:04 PST
Created attachment 17776 [details]
Patch to deal with EINTR

This is the patch I'm currently using to deal with the EINTR case which is very commonly occuring with plug-ins requesting data through the browser.
Comment 14 Darin Adler 2007-12-07 12:08:59 PST
Comment on attachment 17776 [details]
Patch to deal with EINTR

r=me
Comment 15 Rodney Dawes 2007-12-18 11:27:39 PST
Created attachment 17976 [details]
Updated patch with ChangeLog

Here's an updated patch that fixes tabs v. spaces in the code, and includes a ChangeLog entry. Can we please get this committed? Thanks.
Comment 16 Alp Toker 2007-12-18 13:53:19 PST
Landed in r28834.
Comment 17 Alp Toker 2007-12-18 14:53:12 PST
Comment on attachment 17976 [details]
Updated patch with ChangeLog

r=darin