[GitHub] [commons-vfs] kinow commented on pull request #154: Rework SoftRefFilesCache locking
kinow commented on pull request #154: URL: https://github.com/apache/commons-vfs/pull/154#issuecomment-774442260 >If that makes it easier for you, OK for me. Here's part 1: #158 >As soon as you merge it, I'll rebase this PR and drop the merged commits. Thanks @MaxKellermann ! >In projects I maintain, if I like some obvious-good commits, I cherry-pick them and then start discussing the others, and I expect the submitter to rebase the PR with only the unmerged commits remaining. :+1: >What I certainly would never do is fold all PR commits into one large commit like you did with #155 - 3 commits were folded into one (9c4302e). That loses a lot of context, makes the resulting commit hard to read/verify/understand, and makes git-bisect very hard. That's your project, your maintenance decision, so I'm not going to argue, just mentioning it because that's one thing that would make project management hard for me. My workflow is normally to check out the branch locally, rebase onto master — or make sure it's rebased — and then checkout master and merge with `--no-ff`. If the PR has many commits, I may squash them if they appear to be draft/WIP commits, or ask the user if s/he can squash it (asked last week I think, in Apache Commons Imaging). But sometimes we may squash without thinking/asking. Some times when I create a PR I mention that I haven't squashed for some specific reason, and tell the maintainers I'd be happy to squash it if they think it's necessary. If you mention that I'm sure the final reviewer will consider it too. Some projects also have a hard-policy to always merge a single commit (Apache OpenNLP for example, though not sure if that changed). Finally, I forgot to say **thank you** for the great commit messages. They are short, and explain what's included in that commit :+1: This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-vfs] kinow commented on pull request #154: Rework SoftRefFilesCache locking
kinow commented on pull request #154:
URL: https://github.com/apache/commons-vfs/pull/154#issuecomment-774422095
>I totally sympathise with @garydgregory about the lack of test, but also
with @MaxKellermann in that writing tests that exercise thread-safety issues is
a PITA.
Ditto here. Writing tests for the thread-safety would be nice, but
understandable if it's too hard to write.
@MaxKellermann, @garydgregory what do you think if we break this PR into a
few other smaller PR's, I think that would be easier to review, and grouping
that way in case some bugs/issue is raised later, it might be easier to look at
the changelog and see what could be causing it.
For example, I think we could have one PR with smal improvements like (easy
to review, don't think we need tests):
- remove `super.close()` (maybe add a comment to the code where it was
removed so we don't accidentally add it back? Will leave a comment there...)
- use isEmpty() instead of size < 1
- remove redundant isInterrupted() check
- simplify the InterruptedException catch block
Another for the Lock issues (great catch! in some cases we locked twice, in
others forgot to require the lock, and in other cases we also locked the Lock
object without acquiring the lock; I'd say we could code-review this change
without a test [testing synchronized code blocks is hard, but code that is
acquiring a Lock is even harder IMO])
- fix ReentrantLock usage in {start,end}Thread()
- add missing lock to removeFile()
- require lock while calling removeFile(FileSystemAn…
- require lock while calling getOrCreateFilesystemCa…
- move endThread() call inside the lock
- require lock for startThread(), endThread()
- move code to removeFile(Reference)
- don't use ConcurrentMap (oh, I liked this! Would need to read the code
with calm to understand if there was no other case for the concurrent map, but
I think you might be correct here!)
Another PR for the volatile change (code-review might do I think)
- eliminate "volatile" on softRefReleaseThread
And one more for the removal of `ReentrantLock`? Or maybe just create an
issue for discussion regarding the Loom project with fibers and possible issues
(I think that's what @efge meant?).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
