Support symlink webapp deployment

[gregw]

Creating a symlink like webapps/root->/some/path/myapp.war results in a context being deployed at /myapp rather than at /

[joakime]

How does that work?
A directory symlink to a file?

[joakime]

In Jetty 9.3.x a symlink of webapps/bar -> /full/path/to/foo.war is never seen/loaded as a webapp.
Either as /bar or /foo.
Its just not picked up by the WebAppProvider as valid for any reason.

Seems to be because ...

webapps/bar is seen as a "file", which doesn't end in .war, so its not picked up.

[gregw]

Ooops I meant webapps/root.war -> /some/path/foobar.war or webapps/root/ -> /some/path/foobar/

[joakime]

A testcase like this ....

webapps/bar.war -> /full/path/to/foo-123.war

In Jetty 9.3.x, the venerable Scanner is used for WebAppProvider (not the recently added PathWatcher in Jetty 9.4.x)

The Scanner gets the canonical path for the found files before it notifies the WebAppProvider of the added file. By the time the WebAppProvider.fileAdded() is notified, all information about the original link name is lost.

See: https://github.com/eclipse/jetty.project/blob/jetty-9.3.15.v20161220/jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java#L563

I would consider a fix to Scanner to never canonicalize what it finds/reports, but I fear that would be high-risk (as Scanner is in use in a fair number of places)

@gregw how do you want to proceed with this?

[joakime]

Updated (and Ignored) the WebAppProviderTest for working with symlinks.

That test does not pass current (hence the Ignored state)
But if you want to look at this behavior yourself, then remove the Ignore and try yourself.

[gregw]

I think there are two things to consider: updating to use the PathWatcher, and then making it an option to provide either the non canonical path; or both the canonical and the non-canonical path.

[joakime]

Option #3 - no code change, we could document this scenario and say to use a Jetty XML deployable instead.

[gregw]

I think option 3 is what we go with until there is a reason to touch this code (ie to retire Scanner) - hence it is a low priority enhancement.

[adrianmsmith]

This used to work in Jetty 6, admittedly that was a long time ago.

[joakime]

Jetty 6 is EOL and had a few filesystem vulnerabilities when working with various FileSystem quirks (symlinks, hardlinks, alternate data channel access, networked filesystem mounts, etc.)

We really do not want to go back the wild west of unintended behaviors that existed in Jetty 6.

[gregw]

I think we still leave this open. Using a symlink to indicate what the context path should be is a reasonable approach- it's just that our implementation within the Scanner make it hard to do safely or at least easily.

I'll try to have a look in the next few days to really see if there is not an easy/safe way of seeing the link name and using it as the basis of the default context path calculations.

[gregw]

I had a look at the Scanner and it is indeed too difficult to bring back that behaviour.
However, as we have jetty-10 coming out soon, now is exactly the time that we should replace Scanner with PathWatcher and give ourselves the flexibility to be able to do this.

So I've added this a 10 milestone. If we implement it and it proves to be fully backwards compatible, we may cherry pick back to a 9 release.

[gregw]

I've updated the subject to better indicate what this issue has become.
@joakime I've started looking at the PathWatcher with regards to using it for the DeploymentManager, but there are several issues that need to be resolved first, which I'd like to get your feedback on so I don't break any existing usages of the PathWatcher.

Firstly the PathWatcherTest is ignored! I can see why some edge cases might be hard to test cross platform, but we need to have the basic core behaviour tested. I can see one test bug that probably has caused grief with the unit tests: the countdownLatch is decremented and then the results map is updated, so there is a race with waiting for the latch and checking the results. With this swapped, I'm getting all the tests to pass on my machine, plus a new test that I have added.

The new test I have added is just a sequence of pretty normal events, as I wanted to understand the behaviour as it exists. This has revealed a few behaviours that I don't think are completely correct:

3eadc5f#diff-b6fb799345688b3d57231a9689dedc6cR362
When adding/deleting a file to a directory only generates an ADDED/DELETED event for the file and not a MODIFIED event for the directory. But if we are not scanning that directory, then I think we should see the MODIFIED event, but we do not. Note that if we touch a directory without adding/deleting files then we do see MODIFIED event. So I think the ADDED/DELETED events should suppress the MODIFIED event for their directories, but if there are no such events (eg because of depth limit), then we should see the MODIFIED event.

3eadc5f#diff-b6fb799345688b3d57231a9689dedc6cR419
If we slowly modify a file at a rate faster than the quiet time, we see only a single MODIFIED event - yay! But if we slowly add a file at a rate faster than the quiet time, we see both ADDED and MODIFIED events. This is at odds with quickly adding a file where we only see a single ADDED event.

3eadc5f#diff-b6fb799345688b3d57231a9689dedc6cR433
When deleting a directory, we only get a single DELETED event for the directory and not for the files and directories that it contains. This is asymmetric to the multiple ADDED events we get when we add a directory.

For DeploymentManager, fixing the slow add is important, but I'd like to look at fixing the other issues as well. What are your thoughts on them and what other usages of PathWatcher am I likely to break if I start messing with the behaviour too much?

@janbartel as the author if Scanner (albeit a long time ago), I'd appreciate your thoughts on these behaviours as well

[gregw]

@joakime can you test branch jetty-9.4.x-1200 on windows to see if the unit tests are passing?

[joakime]

@gregw sent you the build/test results. 10 failures in OSX 10.11.6 (Java 8u144), 5 failures in Windows 10 (Java 8u144).

[joakime]

Results:

Windows 10 (Java 8u144)

jetty-util : no failures on this branch.
jetty-security: failed test cases. filed as Issue #1789

OSX 10.11.6 (Java 8u144)

jetty-util: failed test cases.

[INFO] Running org.eclipse.jetty.util.PathWatcherTest
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testStartupFindFiles()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testDeployFiles_Update_Delete()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testDeployFiles_ModifyWar_LargeSlowCopy()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testDeployFilesBeyondDepthLimit()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testDeployFiles_NewDir()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testDeployFiles_NewWar()
{foo.war=[ADDED], bar/WEB-INF/web.xml=[ADDED], hello.war=[ADDED, MODIFIED]}
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testGlobPattern()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testWatchFile()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testRestart()
[AdvancedRunner] Running org.eclipse.jetty.util.PathWatcherTest.testSequence()
[ERROR] Tests run: 10, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 340.266 s <<< FAILURE! - in org.eclipse.jetty.util.PathWatcherTest
[ERROR] testDeployFiles_NewWar(org.eclipse.jetty.util.PathWatcherTest)  Time elapsed: 28.019 s  <<< FAILURE!
java.lang.AssertionError:
Events for path [hello.war]
Expected: iterable containing [<ADDED>]
     but: Not matched: <MODIFIED>
	at org.eclipse.jetty.util.PathWatcherTest.testDeployFiles_NewWar(PathWatcherTest.java:744)

[gregw]

@joakime can you run the OSX test again but with logging including:

org.eclipse.jetty.util.PathWatcher.LEVEL=DEBUG

[gregw]

Note that I have merged to jetty-9.4.x branch with the OSX failure ignored as I think this is already better than the untested. We can improve in the main branch

[joakime]

Working with Jesses to get OSX slave added to Jenkins (and update/reconfigure Windows slave) to help test this.

[gregw]

The PathWatcher has never delivered on it's promise of efficient path scanning, primarily because of too many OS differences. PR #4239 has improved the Path semantics of the Scanner, so we no longer need to switch to the PathWatcher.