ExpireAfter Redesign #868
ExpireAfter Redesign #868
Conversation
|
Well, that's fun. That failing test literally ran 1,000 iterations in a row, last night. It's still passing very reliably, in fact. It must be that the hard-coded 100ms wait isn't enough to finish all expirations, when the server is running the full test suite. I'm gonna have to figure out how to signal when the last item actually expires, and put like a 60 second timeout on that. |
JakenVeina
force-pushed
the
bug/expire-after-skipping-items
branch
from
1c297ec
to
f73153d
Compare
|
That took care of it. |
| @@ -424,10 +424,10 @@ dotnet_diagnostic.SA1508.severity = error | |||
| dotnet_diagnostic.SA1509.severity = error | |||
| dotnet_diagnostic.SA1510.severity = error | |||
| dotnet_diagnostic.SA1511.severity = error | |||
| dotnet_diagnostic.SA1512.severity = error | |||
There was a problem hiding this comment.
What are these? If they're important, don't just turn them off for the entire project. Add an exception for each case so that people still try to avoid them if they can.
There was a problem hiding this comment.
SA1512 is new lines after a comment. You can use a SuppressMessageAttribute to disable where its appropriate but generally you don't want new lines after comments anyway.
There was a problem hiding this comment.
Let's see...
RCS1085 is for always using auto-implemented properties, when possible. Violations are here, here, and here. In each of these, we have a complex object where there's far more value in being able to see all of its persistent state (I.E. private fields) in one place, than in reducing the file size by 2 lines. This is a common-enough scenario, in my mind, that it warrants disabling the rule.
SA1201 says "a property should not follow a method", but it ignores the fact that it's a protected property, and I've always encountered standard practice to be that protected instance members don't come before public instance members. Violations are here, here, and here.
SA1512 says "single-line comments should not be followed by blank line", with violations here, here, here, and here. Intuitively, it's standard practice to "attach" comments to the lines of code that they apply to, which I imagine is the origin of this rule, but all of these are examples of comments that DON'T apply to a specific line of code. They are actually commenting on the LACK of lines of code that one might expect to be here. Since the rule applies to a command, and you can't disable rules upon individual comments, I think the rule should be disabled entirely.
SA1513 says "Closing brace should be followed by blank line", and is violated here and here. In these cases, I'm attaching a line of code to the end of a loop, in an effort to indicate that it fundamentally is a finalization action for that loop. Not a big deal, I probably would have just fixed these two if I hadn't already disabled the rule for the 11 other violations (I'll just link this one, they're all the same), which consist of having a break; immediately following a bracketed case block. In my mind the closing bracket itself serves the purpose that a blank line normally would, for a multi-line case block, and inserting an additional blank line here makes it subtly harder to read.
SA1515 says "Single line comments should be preceeded by blank line", and is violated here, here, here, and here. Also here and here, but those are within the .Tests project. In all of these cases, we're commenting on a single line withing a multi-line action, where the comment is too long to put at the end of the line, and where the multi-line action would normally have no blank lines in it. These are all valid in my mind, because of the implicit rule that comments should be directly attached to the thing they're commenting on, not potentially many lines away from it, and with so many valid use-cases being flagged, this seems like a rule that should be disabled.
There was a problem hiding this comment.
I got to be honest, I really dislike style enforcement.
I have been tempted to turn it off entirely in DD as it drives me insane how slow it is to run and how it tries to force me to comment code which I have not even written yet!!!! I always end up turning it off, do my code, then turn it on to see what it complains about. The worst thing is that it also slows my development down.
We're grown ups and surely we should be allowed to express our own style. There are a few things that are important like comments, but surely we could enforce that as part of the PR process.
In a better version of of the net eco-system we'd all be using automatic linting.
Perhaps we should get rid of it completely - thoughts?
There was a problem hiding this comment.
I got to be honest, I really dislike style enforcement.
I fully agree. I think style-enforcement is kind of a fundamentally-flawed concept. Good styling is about promoting human-readability of code, computers are fundamentally ill-equipped to understand what is "human readable", across the board. At least, I have yet to see any system that implements rules granular enough to promote readability, instead of harm it.
Perhaps we should get rid of it completely - thoughts?
I wouldn't lose any sleep, personally. But on a large collaborative project, logically, I know there's a bigger picture than just my opinions, and it would probably be a mistake to remove completely. I think we can focus on de-tuning it, and reducing the impact it has on the developer experience. There are some rules that are nearly-universal, like member ordering and naming, and we gain value by having those. I would point to whitespace rules in particular as usually being best left to a judgement call of the writer.
I always end up turning it off, do my code, then turn it on to see what it complains about.
What's the quick-and-easy way to do this? I've just been manually tossing out #pragma warning disable as they pop up, and it's quite annoying.
we'd all be using automatic linting.
So, wait, what is linting if not this? Like, automatically fixing violations, instead of just reporting them?
| return; | ||
|
|
||
| // Putting the entire management process here inside a .Edit() call for a couple of reasons: | ||
| // - It keeps the edit delegate from becoming a closure |
There was a problem hiding this comment.
It's still a closure though. It captures all the members variables... Which might compile to just capturing this, but it captures something.
We should look at putting a TState<> parameter to the edit method like you suggested.
There was a problem hiding this comment.
It's not QUITE the same as a closure, because like you said, the only state that gets captured is this. Unless I'm much mistaken, what this compiles to is just moving the delegate to an instance method on the class, and then the delegate becomes an allocation per-instance of the class, rather than per call to .Edit(). IIRC, this was discussed in a blog post discussing general delegate enhancements in .NET 6 and .NET 7. Yes, parameterizing the state to make it a static delegate would be even better.
There was a problem hiding this comment.
This is the article I was referring to, and after re-reading, and doing a little ILSpy, I confirmed you're correct. There's no closure, but there is still an allocation, per call to .Edit(). Fixing now...
| @@ -424,10 +424,10 @@ dotnet_diagnostic.SA1508.severity = error | |||
| dotnet_diagnostic.SA1509.severity = error | |||
| dotnet_diagnostic.SA1510.severity = error | |||
| dotnet_diagnostic.SA1511.severity = error | |||
| dotnet_diagnostic.SA1512.severity = error | |||
There was a problem hiding this comment.
I got to be honest, I really dislike style enforcement.
I have been tempted to turn it off entirely in DD as it drives me insane how slow it is to run and how it tries to force me to comment code which I have not even written yet!!!! I always end up turning it off, do my code, then turn it on to see what it complains about. The worst thing is that it also slows my development down.
We're grown ups and surely we should be allowed to express our own style. There are a few things that are important like comments, but surely we could enforce that as part of the PR process.
In a better version of of the net eco-system we'd all be using automatic linting.
Perhaps we should get rid of it completely - thoughts?
…eAfter tests. Reworked stress-testing for ExpireAfter operators, to better ensure that multi-threading contentions are occurring. Added test coverage for "Moved" changes, on the cache stream version of ExpireAfter. Added test coverage for "RemoveRange" changes, on the list version of ExpireAfter.
…e and include actual expiration behavior. Really, just copied the stress-testing code from tests. Also bumped the .Benchmarks project to .NET 8, to match .Tests.
JakenVeina
force-pushed
the
bug/expire-after-skipping-items
branch
from
f73153d
to
9425f24
Compare
…y of defects, including #716.
JakenVeina
force-pushed
the
bug/expire-after-skipping-items
branch
from
9425f24
to
7d2db60
Compare
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Code coverage
Benchmarks
ExpireAfter_Cache_ForSource
ExpireAfter_Cache_ForStream
ExpireAfter_List
(Prior version deadlocks for this benchmark)