Various fixes and improvements to background_fetch #4853
Conversation
|
This still needs some more testing, so don't land yet. I wanted to see that it compiles on all compilers / platforms, and also get some initial input. |
|
I made a few changes / fixes, and I think this is ready for review now. I've tested it with both global and remap plugins, and various configurations etc. |
|
Should you remove the WIP label then? |
SolidWallOfCode
left a comment
SolidWallOfCode
left a comment
There was a problem hiding this comment.
Looks good, except for one major blemish.
| #include <string> | ||
| #include <unordered_map> | ||
| #include <cinttypes> | ||
| #include <string_view> |
There was a problem hiding this comment.
Was that on purpose? By Turing's right patella, you've changed.
| gConfig->readConfig(optarg); | ||
| break; | ||
| } | ||
| if (!gConfig) { |
There was a problem hiding this comment.
Is this checked because of potential remap use? Would it be reasonable to assume that if TSPluginInit is called at all, it's called before any remap rules?
There was a problem hiding this comment.
It’s a remnant from something stupid, I’ll remove it.
| TSDebug(PLUGIN_NAME, "Initialized"); | ||
| TSHttpHookAdd(TS_HTTP_READ_RESPONSE_HDR_HOOK, cont); | ||
| } else { | ||
| // ToDo: Hmmm, no way to fail a global plugin here? |
There was a problem hiding this comment.
OTSOTPR, but this has come up before. We need to do something for that.
|
|
||
| *ih = (void *)config; | ||
| if (success) { | ||
| *ih = (void *)config; |
There was a problem hiding this comment.
I'm actually surprised a cast is needed, as a pointer of any type should auto convert to a void*.
There was a problem hiding this comment.
I don’t remember, the code was like that before :)
|
What’s the major blemish? |
|
Using a C style cast. |
|
Gah, that’s the way it was ... I didn’t add it! But, I’ll go through the entire plugin and fix it up. |
|
You just need to do it on code you touched. |
|
Updated with the review comments, thanks! |
These changes include:
1) Adds a new option, --allow-304 / -a, which allows 304 responses
to still kick off a background fetch. This is important for request
that comes in with e.g.
Range: bytes=0-12
If-Modified-Since: Tue, 22 Jan 2019 19:36:03 GMT
2) Similar, to make the conditional request succeed properly with
conditional requests, we have to filter out more headers. In addition
to the Range: header, we now also filter out any values of
If-Match
If-Modified-Since
If-None-Match
If-Range
If-Unmodified-Since
3) To make all of this work with both global and per-remap setups,
we generalize the option parsing to work equally for both types. To
be backward compatible, a single option without a leading '-' is
assumed to be the name of the configuration file, but the correct
way to use this in a remap plugin is
@pparam=--config=/some/config/file.conf
4) In addition, while reading up on this, we should allow the
background fetch to be kicked off with an If-Range header.
|
Cherry-picked to 8.0.x |
3 participants
These changes include:
Adds a new option, --allow-304 / -a, which allows 304 responses
to still kick off a background fetch. This is important for request
that comes in with e.g.
Range: bytes=0-12
If-Modified-Since: Tue, 22 Jan 2019 19:36:03 GMT
Similar, to make the conditional request succeed properly with
conditional requests, we have to filter out more headers. In addition
to the Range: header, we now also filter out any values of
If-Match
If-Modified-Since
If-None-Match
If-Range
If-Unmodified-Since
To make all of this work with both global and per-remap setups,
we generalize the option parsing to work equally for both types. To
be backward compatible, a single option without a leading '-' is
assumed to be the name of the configuration file, but the correct
way to use this in a remap plugin is
@pparam=--config=/some/config/file.conf
In addition, while reading up on this, we should allow the
background fetch to be kicked off with an If-Range header.