[WIP] Implement a maximum response size option #902
Conversation
Codecov Report
@@ Coverage Diff @@
## master #902 +/- ##
==========================================
+ Coverage 87.67% 88.59% +0.92%
==========================================
Files 41 41
Lines 5726 5807 +81
Branches 791 807 +16
==========================================
+ Hits 5020 5145 +125
+ Misses 525 482 -43
+ Partials 181 180 -1
|
Gallaecio
force-pushed
the
max-response-size
branch
5 times, most recently
from
1b31cc6
to
8cf3788
Compare
Gallaecio
force-pushed
the
max-response-size
branch
from
8cf3788
to
472237d
Compare
Gallaecio
force-pushed
the
max-response-size
branch
2 times, most recently
from
c4d033a
to
a836917
Compare
Gallaecio
force-pushed
the
max-response-size
branch
from
a836917
to
9ed7714
Compare
| return value | ||
|
|
||
|
|
||
| def _size_warrants_abort(sizes_and_sources, render_options, log, reply): |
There was a problem hiding this comment.
this function looks over-complicated, can we improve it somehow?
There was a problem hiding this comment.
Maybe I can extract some of the inner logic into separate functions, such as the parsing of max_size.
| @@ -6,6 +6,9 @@ | |||
|
|
|||
| MAX_TIMEOUT = 90.0 | |||
|
|
|||
| RESPONSE_SIZE_LIMIT = None | |||
| MAX_RESPONSE_SIZE_LIMIT = None | |||
There was a problem hiding this comment.
This two looks very alike so a bit confusing, please add a comment about what each they stands for?
BTW why do we need two variables for this limit?
There was a problem hiding this comment.
If I recall right:
-
RESPONSE_SIZE_LIMITis the limit to use as default if the Splash request does not specify one. -
MAX_RESPONSE_SIZE_LIMITis the maximum value that a Splash request may specify. If a higher value is specified by a Splash request,MAX_RESPONSE_SIZE_LIMITis used as limit instead.
It is indeed confusing, I should indeed add a comment.
| @@ -413,6 +502,16 @@ def _on_reply_headers(self): | |||
| self.log("Headers received for {url}", reply, min_level=3) | |||
|
|
|||
| def _on_reply_download_progress(self, received, total): | |||
| reply = self.sender() | |||
| request = reply.request() | |||
| @@ -86,6 +153,7 @@ class ProxiedQNetworkAccessManager(QNetworkAccessManager): | |||
| * Tracks information about requests/responses and stores it in HAR format, | |||
| including request and response content. | |||
| * Allows to set per-request timeouts. | |||
| * Handles per-request response size limits. | |||
There was a problem hiding this comment.
I don't find how to set the "per-request response size limit" in this PR.
There was a problem hiding this comment.
By adding max_size to the render options of a request.
If you mean that the documentation should explain this, indeed.
There was a problem hiding this comment.
Thanks for the early feedback!
However, I don’t think I’ll complete it any time soon, my priorities have switched away from Splash for the time being. If anyone finds this pull request and is interested in completing it, feel free to fork it on a new pull request, and leave a comment about it here.
| @@ -6,6 +6,9 @@ | |||
|
|
|||
| MAX_TIMEOUT = 90.0 | |||
|
|
|||
| RESPONSE_SIZE_LIMIT = None | |||
| MAX_RESPONSE_SIZE_LIMIT = None | |||
There was a problem hiding this comment.
If I recall right:
-
RESPONSE_SIZE_LIMITis the limit to use as default if the Splash request does not specify one. -
MAX_RESPONSE_SIZE_LIMITis the maximum value that a Splash request may specify. If a higher value is specified by a Splash request,MAX_RESPONSE_SIZE_LIMITis used as limit instead.
It is indeed confusing, I should indeed add a comment.
| return value | ||
|
|
||
|
|
||
| def _size_warrants_abort(sizes_and_sources, render_options, log, reply): |
There was a problem hiding this comment.
Maybe I can extract some of the inner logic into separate functions, such as the parsing of max_size.
| @@ -86,6 +153,7 @@ class ProxiedQNetworkAccessManager(QNetworkAccessManager): | |||
| * Tracks information about requests/responses and stores it in HAR format, | |||
| including request and response content. | |||
| * Allows to set per-request timeouts. | |||
| * Handles per-request response size limits. | |||
There was a problem hiding this comment.
By adding max_size to the render options of a request.
If you mean that the documentation should explain this, indeed.
Based on #58.