9138 hostname caching policy #793
Conversation
|
@jlitzingerdev Python 3 seems to be failing :( |
|
Uy, Indeed it is, sorry about. My process was broken, I didn't test python 3 with TLS dependencies installed...process fixed. I'm assuming I should close, rebase, and submit a new PR, correct? Do I need to version (thinking of patch versions for kernel submissions) the PR or is a new commit sufficient? |
|
@jlitzingerdev Nah, just go ahead and push another commit to the branch/PR, and it'll update/rerun tests. |
Codecov Report
@@ Coverage Diff @@
## trunk #793 +/- ##
==========================================
+ Coverage 91.92% 91.92% +<.01%
==========================================
Files 845 845
Lines 151151 151224 +73
Branches 13187 13192 +5
==========================================
+ Hits 138940 139015 +75
+ Misses 10113 10111 -2
Partials 2098 2098 |
|
@hawkowl Pushed, though it ended up being three commits as there were three distinct issues I found with the previous submission. I did not rebase on trunk, but will be happy to do so if needed. |
|
@hawkowl I must admin I'm a bit confused on the travis failure. Most of the jobs passed, and the one that failed "ERROR: manifest-checker: commands failed", is this related to my branch being out of date with master? |
|
You should merge trunk into your branch to update it, https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch Your lack of a news fragment is causing the Travis failure. |
|
@rodrigc I did indeed read the newsifle requirement, and I added a one, see https://github.com/twisted/twisted/pull/793/files#diff-3d267930e0599d705b7ed77fd7572f25, though I added it in the newsfragments directory of the twisted.web package, was that incorrect? |
|
@rodrigc Also, copy that on the merge of trunk, but a question, do you really want a merge, or should I rebase my changes on current trunk (so this PR is always a fast forward)? My only concern with a merge is that, assuming I have more feedback to incorporate, new changes for this changeset will be spread across a merge commit (somewhat pointlessly). Ultimately I'll do whatever is preferred, just want the history to look how want it. |
|
Merge is fine. |
|
@rodrigc thanks, one last question pre-push. Did you see my comment about the newsfile? I did add one, and the build did pick it up. It appears the build is complaining about Which I'm assuming will be resolved with the merge. Was there an issue with my newsfragment as submitted? |
|
Yes, I saw the newsfragment you submitted. Please do the merge. |
…ing-policy
Merge of trunk as requested during review of the pull request for ticket
9138.
See:
twisted#793
For the full discussion.
|
Your branch has a file src/twisted/names/topfiles/8340.bug fix but that file does not exist on trunk. Maybe there was a conflict during a merge and you didn't resolve it correctly? |
|
You need to update your tree at https:://github.com/jlitzingerdev/twisted so that it's trunk has all the commits from https://github.com/twisted/twisted . You are missing commits done later this week which are needed. Then you need to do another merge from trunk to this branch. |
|
@rodrigc That makes more sense. I haven't kept my fork's trunk up to date (it is locally, but I didn't push), I was (wrongly) assuming it didn't matter as the PR was against twisted's. Sorry, one moment. |
|
@rodrigc Done, fork trunk updated, sorry about that. Re. your other comment, that 8430 does appear to exist in twisted's trunk, but it did not exist in mine at the time. Can I assume that the tests, when they need trunk use the fork's trunk? If so, would that be helpful to add to the documentation (unless I just missed it)? If so I can add it. Also, I did merge my fork's trunk into this branch, but it was already up to date a I merged trunk from the upstream earlier (git merge upstream/trunk). |
|
@markrwilliams you introduced src/twisted/names/topfiles/8340.bugfix, which is now in an incorrect directory. This is now causing other pull requests to fail. Can you fix this? |
|
@rodrigc I can fix it in this case, but @hawkowl wrote a very handy script: |
|
I've updated the wiki |
There was a problem hiding this comment.
@markrwilliams Can you cherry pick 98d3e64 to trunk? The travis trunk build is red without this fix: https://travis-ci.org/twisted/twisted/branches
|
@rodrigc @hawkowl @markrwilliams It looks like the latest merge (fedec1a) introduced the same topfile fix. Do you want me to merge trunk again, or create a clean v2 branch/PR with the commits pertinent to ticket 9138 (perhaps squashed back down to two commits)? |
jlitzingerdev
force-pushed
the
9138-hostname-caching-policy
branch
from
8779434
to
74b3b58
Compare
|
I rebased this branch against trunk today. I'd merged trunk in on Sunday, but it was becoming fairly messy. I assumed rebasing this was desirable per item 15 in https://twistedmatrix.com/trac/wiki/TwistedDevelopment#SubmittingaPatch. That unfortunately required a force-push, which I typically avoid, apologies for any issues that causes. Note that I also updated my fork's trunk just after this push, which might trigger a CI failure (it seemed to before). Unless I hear otherwise I'll plan to continue rebasing this against trunk. |
jlitzingerdev
force-pushed
the
9138-hostname-caching-policy
branch
from
74b3b58
to
78f04f0
Compare
The custom trust root is useful when testing custom HTTPS policies, move it outside its current test function if the SSL dependencies are successfully imported.
The HostnameCachingHTTPSPolicy is an IPolicyForHTTPS that caches connection creators for reuse in subsequent requests. The cache size is determined by the cacheSize parameter and defaults to 20. When a cache miss occurs, an entry for the new host is created and added to the cache. The cache is then purged and the oldest entry removed.
The entry.creator was cruft from a refactoring that never should have remained in the branch, remove.
The use of finally in the previous code hid two bugs: 1. If an exception was thrown by optionsForClientTLS, entry would not exist and an UnboundLocalError would be thrown in the finally clause, masking the original exception. 2. If 1 occured, _nextCacheId would still be incremented, leaving a hole and resulting in no cache entry removed during a later purge (due to the fact that no entry existed with that value).
Caught during test-build of pull request. Unicode objects don't have a decode method in Python3. Modify the code such that, when an argument is passed to creatorForNetloc it is always an ascii encoded byte string, but when the _cache is manually inspected a unicode (str) object is used.
Test classes should end with Tests and a python code style bug.
|
This lint failure looks silly; don't have time to do a full review right now, but a committer should probably just come along and pacify the builder and land this. |
|
@glyph I assumed from the age that there wasn't much interest in this enhancement. I haven't done much in the way of maintenance (as evidenced in the logs above) in quite some time. I'll clean up the errors and get it in shape for a proper review. Note that I did submit a PR for the benchmarks (what's the point in an improvement if there is no measurement) here: |
Caught by checker in review.
Caught during review by twisted checker.
|
As an aside, on Python 3, this could also be done using the |
wiml
force-pushed
the
9138-hostname-caching-policy
branch
from
e90b7bd
to
79d8b18
Compare
wiml
force-pushed
the
9138-hostname-caching-policy
branch
from
79d8b18
to
0ecb515
Compare
PR for https://twistedmatrix.com/trac/ticket/9138