Simplified TemporaryDirectoryAllocator.dispose #198

jglick · 2020-01-22T22:17:47Z

Experience in jenkinsci/jenkins#4447 shows that the FilePath- and thus PathRemover-based implementation does not work reliably in Windows CI after #166, and we get stuff like #185 which is making things more complicated than it really needs to be. In a test, we

expect deletion to succeed, so there is no need to collect errors—fail on the first one
do not care to retry deletions like we might in production
do not expect there to be unwritable files

jtnord

looks good to me - but I would generally not require a run of the gc as stated

src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java

slonopotamus · 2020-01-22T22:40:48Z

#166 does not guarantee it will detect a bug. But if it detects something, you may be sure that there is a bug in code. I want to say that "able to detect bugs unreliably" is better than "unable to detect bugs". The point of #166 is not to delete directory. Nobody cares if it is deleted or not, actually! The point is to catch buggy code that forgot to close file descriptors after finishing.

jenkinsci/git-plugin#798, jenkinsci/subversion-plugin#240 and jenkinsci/workflow-support-plugin#99 were all actual bugs in production code of plugins, discovered thanks to #166.

slonopotamus · 2020-01-22T22:42:41Z

If you need some help figuring out what exact code is guilty in failed tests in jenkinsci/jenkins#4447 - I can definitely do that.

…to TemporaryDirectoryAllocator.dispose

jglick · 2020-10-07T20:16:57Z

I see no reason to hold off on this PR just because jenkinsci/jenkins#4447 is stalled.

jtnord · 2020-10-07T23:28:03Z

src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java

+
+    private void delete(Path p) throws IOException {
+        LOGGER.fine(() -> "deleting " + p);
+        if (Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS)) {


probably also a junction which I think may be used in some test case and FIlePath respects.. (FIles.readAttributes and isOther - as a general well it is not a normal directory)

I do not know much about junctions. Would they ever really be used in $JENKINS_HOME?

I'm sure something uses them as we have specific code in Jenkins...
https://github.com/jenkinsci/jenkins/blob/715cdb15b3e855f8b6115aa02200860ed6caf49d/core/src/main/java/hudson/Util.java#L323-L343
I have not come across a real life example, but I'm guessing that it is for a reason?

Maybe it is just an easy way in Jenkins to put some subset of Jobs in a different drive / pool etc and would not actually be done inside a workspace, or a build directory by jenkins (I do not know if zipping on windows copies the Junction or not, so if a workspace was coppied from an agent where the build created a junction if that would be replcicated on a master).

ec2 plugin seems to use them for something?
https://github.com/jenkinsci/ec2-plugin/blob/20f0c2406d62ddbd0d005a2fb15f3f9bb95b3f20/src/main/java/hudson/os/WindowsUtil.java#L109-L121

and the workspacebrowser has a test to prevent traversing them IIRC (so there is at least one use in Jenkins where a workspace will have junctions) https://github.com/jenkinsci/jenkins/blob/9330635821cbcb430b4223b471692f2c9ae862db/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java#L617

There might be some obscure use case in production servers, but I doubt this would affect functional tests. If there is some plugin which does create a junction during a test for some reason, then it can explicitly remove it before generic cleanup, right?

ahh now https://github.com/jenkinsci/jenkins/pull/8006/files#r1199177115 makes sense

src/main/java/org/jvnet/hudson/test/JenkinsRule.java

…rness into TemporaryDirectoryAllocator.dispose

jglick · 2023-05-18T11:28:56Z

Tested successfully in jenkinsci/jenkins#8006 which I think is good enough.

src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java

jtnord · 2023-05-19T16:59:47Z

src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java

+
+    private void delete(Path p) throws IOException {
+        LOGGER.fine(() -> "deleting " + p);
+        if (Files.isDirectory(p, LinkOption.NOFOLLOW_LINKS)) {


ahh now https://github.com/jenkinsci/jenkins/pull/8006/files#r1199177115 makes sense

src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java

…ryAllocator.dispose` change) (#8006) * Testing `TemporaryDirectoryAllocator.dispose` change * `QueueTest.pendingsConsistenceAfterErrorDuringMaintain` should wait for completion of any builds it started * Close a `FileOutputStream` in `LauncherTest.remotable` * `DirectoryBrowserSupportTest.junctionAndSymlink_outsideWorkspace_areNotAllowed_windowsJunction` to clean up junctions it created * Reverting temporary dep on jenkinsci/jenkins-test-harness#198 * Commenting reason for `rmdir /s /q` #8006 (review)

basil · 2023-05-29T17:01:25Z

Caused #591.

basil · 2023-06-06T22:00:34Z

Caused JENKINS-71406.

slonopotamus · 2023-06-07T11:33:14Z

Well, if this change exposes bugs in code, it's not this change fault that other code is buggy.

MarkEWaite · 2023-06-07T14:48:40Z

Well, if this change exposes bugs in code, it's not this change fault that other code is buggy.

I agree that it is not the fault of this change that other code has bugs. However, it seems important to the overall progress of the project that when general behavior (tests passing reliably) regresses, it is better to either fix the problems that have been exposed by the change or revert the change that exposed the problems. Remaining for an extended period in a state where tests are flaky (the problem is exposed but not resolved) will reduce our ability to trust the tests and our ability to add new capabilities.

It is a good thing that we see the problems, but if we can't resolve the problems that we now see (in a reasonable amount of time and effort), then I think it is healthier for the project to return to hiding the problems (by reverting the change) rather than slow further progress with the distraction of seeing those problems.

jglick · 2023-06-07T19:17:57Z

Commented in Jira. Please keep discussion there.

basil · 2023-06-21T15:25:10Z

Caused #612.

basil · 2023-08-19T15:02:20Z

@jglick This PR is causing problems in https://github.com/jenkinsci/copyartifact-plugin

jglick · 2023-08-21T15:37:28Z

jenkinsci/copyartifact-plugin#205 👀

basil · 2023-09-14T19:31:02Z

@jglick This PR is causing problems in jenkinsci/workflow-cps-global-lib-http-plugin#167

Simplified TemporaryDirectoryAllocator.dispose

6c30c1f

jglick requested a review from jtnord January 22, 2020 22:17

Forgot some debug code.

ee7b90a

jtnord approved these changes Jan 22, 2020

View reviewed changes

src/main/java/org/jvnet/hudson/test/TemporaryDirectoryAllocator.java Outdated Show resolved Hide resolved

jglick added a commit to jglick/jenkins that referenced this pull request Jan 22, 2020

https://github.com/jenkinsci/jenkins-test-harness/pull/198

3e09407

jglick added 5 commits January 22, 2020 21:24

Do not GC

8ae59ec

Did not notice that DirectoryStream is in fact Iterable

99bee8b

Better diagnosis for DirectoryNotEmptyException

ee72ff5

This GC call should also be unnecessary

52845e4

Merge branch 'master' of github.com:jenkinsci/jenkins-test-harness in…

0042f34

…to TemporaryDirectoryAllocator.dispose

jglick mentioned this pull request Jul 20, 2020

JENKINS-62739 Remove Bootstrap CSS jenkinsci/cloudbees-jenkins-advisor-plugin#60

Merged

jglick mentioned this pull request Oct 7, 2020

Allow SurefireArchiverUnitTest to run in PCT against newer junit jenkinsci/maven-plugin#143

Merged

Merge branch 'master' of github.com:jenkinsci/jenkins-test-harness in…

1350354

…to TemporaryDirectoryAllocator.dispose

jglick marked this pull request as ready for review October 7, 2020 20:15

jglick added the enhancement label Oct 7, 2020

jglick requested a review from jtnord October 7, 2020 20:17

jtnord reviewed Oct 7, 2020

View reviewed changes

slonopotamus reviewed Oct 8, 2020

View reviewed changes

src/main/java/org/jvnet/hudson/test/JenkinsRule.java Outdated Show resolved Hide resolved

jglick marked this pull request as draft October 12, 2020 15:34

Merge branch 'master' of https://github.com/jenkinsci/jenkins-test-ha…

f3625f0

…rness into TemporaryDirectoryAllocator.dispose

jglick mentioned this pull request May 16, 2023

Fixing file leaks in tests (preadapting to proposed TemporaryDirectoryAllocator.dispose change) jenkinsci/jenkins#8006

Merged

6 tasks

Merge branch 'master' into TemporaryDirectoryAllocator.dispose

c2c92fe

jglick marked this pull request as ready for review May 18, 2023 11:29

jglick requested a review from jtnord May 18, 2023 11:29

jglick added a commit to jglick/jenkins that referenced this pull request May 18, 2023

Reverting temporary dep on jenkinsci/jenkins-test-harness#198

64e64cc

jtnord approved these changes May 19, 2023

View reviewed changes

Avoid Guava jenkinsci#198 (comment)

de89ed2

jglick enabled auto-merge May 19, 2023 18:27

jglick merged commit 4f7e40a into jenkinsci:master May 19, 2023

jglick deleted the TemporaryDirectoryAllocator.dispose branch May 30, 2023 11:30

jglick mentioned this pull request Jun 9, 2023

[JENKINS-71406] Flake in NodeCanTakeTaskTest.becauseNodeIsBusy jenkinsci/jenkins#8115

Merged

6 tasks

This was referenced Aug 16, 2023

Three tests are flaky jenkinsci/gitlab-plugin#1563

Merged

Require Jenkins 2.387.3 or newer jenkinsci/copyartifact-plugin#203

Closed

jglick mentioned this pull request Aug 23, 2023

Adding RemainingActivityListener for diagnosis #643

Merged

jglick mentioned this pull request Sep 15, 2023

Work around an apparent JDK file deletion bug on Windows #654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplified TemporaryDirectoryAllocator.dispose #198

Simplified TemporaryDirectoryAllocator.dispose #198

jglick commented Jan 22, 2020 •

edited

Loading

jtnord left a comment

slonopotamus commented Jan 22, 2020

slonopotamus commented Jan 22, 2020 •

edited

Loading

jglick commented Oct 7, 2020

jtnord Oct 7, 2020

jglick Oct 7, 2020

jtnord Oct 8, 2020 •

edited

Loading

jglick May 16, 2023

jtnord May 19, 2023

jglick commented May 18, 2023

jtnord May 19, 2023

basil commented May 29, 2023

basil commented Jun 6, 2023

slonopotamus commented Jun 7, 2023

MarkEWaite commented Jun 7, 2023 •

edited

Loading

jglick commented Jun 7, 2023

basil commented Jun 21, 2023

basil commented Aug 19, 2023

jglick commented Aug 21, 2023

basil commented Sep 14, 2023

Simplified TemporaryDirectoryAllocator.dispose #198

Simplified TemporaryDirectoryAllocator.dispose #198

Conversation

jglick commented Jan 22, 2020 • edited Loading

jtnord left a comment

Choose a reason for hiding this comment

slonopotamus commented Jan 22, 2020

slonopotamus commented Jan 22, 2020 • edited Loading

jglick commented Oct 7, 2020

jtnord Oct 7, 2020

Choose a reason for hiding this comment

jglick Oct 7, 2020

Choose a reason for hiding this comment

jtnord Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

jglick May 16, 2023

Choose a reason for hiding this comment

jtnord May 19, 2023

Choose a reason for hiding this comment

jglick commented May 18, 2023

jtnord May 19, 2023

Choose a reason for hiding this comment

basil commented May 29, 2023

basil commented Jun 6, 2023

slonopotamus commented Jun 7, 2023

MarkEWaite commented Jun 7, 2023 • edited Loading

jglick commented Jun 7, 2023

basil commented Jun 21, 2023

basil commented Aug 19, 2023

jglick commented Aug 21, 2023

basil commented Sep 14, 2023

jglick commented Jan 22, 2020 •

edited

Loading

slonopotamus commented Jan 22, 2020 •

edited

Loading

jtnord Oct 8, 2020 •

edited

Loading

MarkEWaite commented Jun 7, 2023 •

edited

Loading