[MRG+1] Added option to disable the webkit in-memory and network caches #821

wsdookadr · 2018-10-07T15:57:08Z

Hi, I've been reading through the existing open issues.
This pull request is intended to address #203 , #519 , scrapy-plugins/scrapy-splash#168 , #817 , webrecorder/har2warc#1

Please review

Thanks,
Stefan

Note: To try this out, you can use this docker image:

docker pull wsdookadr/splash:v1
docker run -it -p 8050:8050  --name splash_v1 wsdookadr/splash:v1 --disable-browser-caches

And then load a web page and generate a .har file from it like so

curl "http://localhost:8050/render.har?url=https://www.digitalocean.com/community/tutorials/how-to-install-java-with-apt-on-debian-9&timeout=13&wait=4&response_body=1" > /tmp/q.har

If you run this multiple times, the amount of data fetched by curl should be about the same (4mb for that url). Before this pull-request, the first time, it would produce around 4mb, and the subsequent ones about ~200-300k (the .har files would be missing resources that are cached by webkit in its page cache, the first time the page was loaded).

cathalgarvey · 2018-10-08T11:25:54Z

On cursory review the code changes are clean and their purpose is clear. Some minor formatting changes to comply with PEP8, and a typo in the documentation changes, but nothing that should delay a merge. I'll +1 it and a make a few inline suggestions; if another maintainer can review and confirm also I expect we can merge it.

cathalgarvey

Two minor changes recommended but should not delay a merge. I think this is a valuable feature for some use-cases and adds a level of control that might be useful generally for experimenting and testing.

It would be nice, if the new functionality could be tested, to include tests for it.

docs/faq.rst

splash/server.py

docs/faq.rst

…e pep8-compatible

codecov · 2018-10-08T23:02:54Z

Codecov Report

❗ No coverage uploaded for pull request base (master@a3f7e75). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #821   +/-   ##
=========================================
  Coverage          ?   87.46%           
=========================================
  Files             ?       41           
  Lines             ?     5670           
  Branches          ?      781           
=========================================
  Hits              ?     4959           
  Misses            ?      533           
  Partials          ?      178

Impacted Files	Coverage Δ
splash/network_manager.py	`85.81% <100%> (ø)`
splash/server.py	`71.06% <100%> (ø)`

kmike · 2018-10-09T15:12:59Z

Hey @wsdookadr! The PR looks good, but could you please add a test case? Check e.g. here how a custom startup option is tested. I guess you'd need to define some resource in

splash/splash/tests/mockserver.py

Line 975 in a3f7e75

class Root(Resource):

with caching behavior, then request it

twice with a Splash started with default options - resource should be in har only for one of them,
twice with a Splash started with --disable-browser-caches - resource should be in har in both cases

wsdookadr · 2018-10-14T12:14:34Z

@kmike I've pushed another commit with tests for this. However, it seems like the problem is not being captured by the test, in other words both default options and the new --disable-browser-caches options will bring all the entries with content in the HAR.
The new test can be run using the following command:

pytest -s --verbosity=6 ./splash/tests/test_request_filters.py -k test_disable_browser_caches

kmike · 2018-10-14T12:35:54Z

splash/tests/mockserver.py

+    @use_chunked_encoding
+    def render_GET(self, request):
+        return ("""<html><head>
+                <link rel="stylesheet" href="subresources-with-caching/style.css" />


I think the problem could be that related resources don't set any specific cache headers, so the browser doesn't cache them. Could you try setting Cache-Control header for these static resources?

@kmike you are correct, I've pushed a new commit that sets that Cache-Control header and now it seems that at least for the image.gif resource, the entire scenario is reproducible.

Used the Cache-Control header to allow the browser to cache the image.gif that is being fetched. This now actually reproduces the behaviour of the browser which would discard the image.gif resource from the HAR if the page/in-memory cache was enabled.

kmike · 2018-10-15T10:31:05Z

splash/tests/test_request_filters.py

+    # pytest -s --verbosity=6 ./splash/tests/test_request_filters.py -k test_disable_browser_caches
+    def test_disable_browser_caches(self):
+        # entries if run with caches enabled
+        # in this case we should have  [[content,content,content],[content,content,no content]]


I wonder why isn't it working for css files. Maybe there are some other response headers needed to make it work, or maybe browser uses ETag or something like that, so there is no "full" response, but still a response. It doesn't look good that our code sets cache headers, but then checks that the resource is not cached; test will break if resource start to be cached.

I think there are two ways to fix that:

Investigate what's going on and make sure css is cached as well;

remove a check for css file, check only html and gif.

I agree with you on that and I went with the 2nd approach, I have removed the css checks. I've pushed a new commit for this.
For this pull request I'm ok with having it this way.

kmike · 2018-10-16T09:29:54Z

splash/tests/mockserver.py

+            return self.Image()
+        return self
+
+    class StyleSheet(Resource):


Can it be removed now, as well as <link rel=stylesheet> tag in html?

kmike · 2018-10-16T11:51:50Z

splash/tests/mockserver.py

@@ -901,6 +901,39 @@ def render_GET(self, request):
            return base64.decodebytes(b'R0lGODlhAQABAAD/ACwAAAAAAQABAAACADs=')


+class SubresourcesWithCaching(Resource):
+    """
+        Embedded css and image.


docstring is no longer correct, as it embeds only image

kmike · 2018-10-16T11:52:34Z

splash/tests/mockserver.py

+    @use_chunked_encoding
+    def render_GET(self, request):
+        return ("""<html><head>
+            </head>


I think you can remove <head>, to make an example a bit shorter.

kmike · 2018-10-16T13:57:18Z

Thanks @wsdookadr! 🎉

Mideen · 2018-12-19T16:30:46Z

Hi team,
When can we expect the next stable version with these changes?
@kmike

added option to disable the webkit in-memory and network caches

7b2ea43

wsdookadr mentioned this pull request Oct 7, 2018

problem after converting a .har -> .warc and importing in webrecorder webrecorder/har2warc#1

Closed

cathalgarvey changed the title ~~added option to disable the webkit in-memory and network caches~~ [MRG+1] Added option to disable the webkit in-memory and network caches Oct 8, 2018

cathalgarvey approved these changes Oct 8, 2018

View reviewed changes

docs/faq.rst Outdated Show resolved Hide resolved

splash/server.py Outdated Show resolved Hide resolved

kmike reviewed Oct 8, 2018

View reviewed changes

docs/faq.rst Show resolved Hide resolved

fixed typo, re-arranged docs to align with docs format, made code mor…

bab90c8

…e pep8-compatible

added tests for PR #821

558d831

kmike reviewed Oct 14, 2018

View reviewed changes

improved tests for PR #821

3c27d1e

Used the Cache-Control header to allow the browser to cache the image.gif that is being fetched. This now actually reproduces the behaviour of the browser which would discard the image.gif resource from the HAR if the page/in-memory cache was enabled.

wsdookadr mentioned this pull request Oct 14, 2018

interoperability with har2warc #817

Closed

kmike reviewed Oct 15, 2018

View reviewed changes

removed style.css tests for now

d9a69ee

kmike reviewed Oct 16, 2018

View reviewed changes

removed everything related to style.css from the tests

b7226c3

kmike reviewed Oct 16, 2018

View reviewed changes

fixed comments, removed <head> tag

ba42957

kmike merged commit 7eb9a3c into scrapinghub:master Oct 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Added option to disable the webkit in-memory and network caches #821

[MRG+1] Added option to disable the webkit in-memory and network caches #821

wsdookadr commented Oct 7, 2018 •

edited

cathalgarvey commented Oct 8, 2018

cathalgarvey left a comment

codecov bot commented Oct 8, 2018 •

edited

kmike commented Oct 9, 2018

wsdookadr commented Oct 14, 2018

kmike Oct 14, 2018

wsdookadr Oct 14, 2018

kmike Oct 15, 2018 •

edited

wsdookadr Oct 16, 2018

kmike Oct 16, 2018

kmike Oct 16, 2018

kmike Oct 16, 2018

kmike commented Oct 16, 2018

Mideen commented Dec 19, 2018

[MRG+1] Added option to disable the webkit in-memory and network caches #821

[MRG+1] Added option to disable the webkit in-memory and network caches #821

Conversation

wsdookadr commented Oct 7, 2018 • edited

cathalgarvey commented Oct 8, 2018

cathalgarvey left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 8, 2018 • edited

Codecov Report

kmike commented Oct 9, 2018

wsdookadr commented Oct 14, 2018

kmike Oct 14, 2018

Choose a reason for hiding this comment

wsdookadr Oct 14, 2018

Choose a reason for hiding this comment

kmike Oct 15, 2018 • edited

Choose a reason for hiding this comment

wsdookadr Oct 16, 2018

Choose a reason for hiding this comment

kmike Oct 16, 2018

Choose a reason for hiding this comment

kmike Oct 16, 2018

Choose a reason for hiding this comment

kmike Oct 16, 2018

Choose a reason for hiding this comment

kmike commented Oct 16, 2018

Mideen commented Dec 19, 2018

wsdookadr commented Oct 7, 2018 •

edited

codecov bot commented Oct 8, 2018 •

edited

kmike Oct 15, 2018 •

edited