Bug 145797 – [SOUP] Network Cache: run the IO completion handler in the given queue instead of the whole operation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

I misunderstood what the WorkQueue parameter meant in the IO channel operations. It's the queue where the completion handler should be run, not the whole operation. Since our operations are already non-blocking, we can just run the read/writes in the main thread, and schedule the completion handler in the given work queue when the operation finishes.

[Carlos Garcia Campos]

Created attachment 254561 [details]
Patch

[WebKit Commit Bot]

Attachment 254561 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:156:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:178:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:265:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 254561 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254561&action=review

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> +            auto asyncData = std::unique_ptr<ReadAsyncData>(asyncDataPtr);

This can be just

    std::unique_ptr<ReadAsyncData> asyncData(asyncDataPtr);

Same for the rest of such statements.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:188
> +            Data data;
> +            completionHandler(data, -1);

You could probably just construct the Data object in the completionHandler call:

    completionHandler(Data(), -1);

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:191
> +        g_cond_wait(&condition, &mutex);

Can this function be called on the main thread, with a null queue?

That would schedule the callback in the main context and continue to block the main thread in g_cond_wait(), with the callback never invoked.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:223
>      g_cond_wait(&condition, &mutex);

Same problem here.

[Carlos Garcia Campos]

(In reply to comment #3)
> Comment on attachment 254561 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254561&action=review
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:119
> > +            auto asyncData = std::unique_ptr<ReadAsyncData>(asyncDataPtr);
> 
> This can be just
> 
>     std::unique_ptr<ReadAsyncData> asyncData(asyncDataPtr);
> 
> Same for the rest of such statements.

Ok, no much difference anyway.

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:188
> > +            Data data;
> > +            completionHandler(data, -1);
> 
> You could probably just construct the Data object in the completionHandler
> call:
> 
>     completionHandler(Data(), -1);

No, that doesn't build.

no known conversion for argument 1 from ‘WebKit::NetworkCache::Data’ to ‘WebKit::NetworkCache::Data&’

> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:191
> > +        g_cond_wait(&condition, &mutex);
> 
> Can this function be called on the main thread, with a null queue?

No this, is always called from a thread. I'll add an assert to ensure it.

> That would schedule the callback in the main context and continue to block
> the main thread in g_cond_wait(), with the callback never invoked.
> 
> > Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:223
> >      g_cond_wait(&condition, &mutex);
> 
> Same problem here.

Yes, we definitely don't want to block the main thread ever.

[Carlos Garcia Campos]

Created attachment 254570 [details]
Updated patch

[WebKit Commit Bot]

Attachment 254570 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:157:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:179:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheIOChannelSoup.cpp:268:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Comment on attachment 254570 [details]
Updated patch

Looks good.

[Carlos Garcia Campos]

Committed r185365: <http://trac.webkit.org/changeset/185365>