Windows shared classes system tests failing, shared cache file path should be less than 249

[pshipton]

The tests were passing, but have started failing with the merge of #4532. Since they were passing previously, perhaps the length check is too restrictive.

JVMSHRC828E The length of shared cache file path should be less than 249.

https://ci.eclipse.org/openj9/job/Test-extended.system-JDK11-win_x86-64_cmprssptrs/162/
https://ci.eclipse.org/openj9/job/Test-extended.system-JDK8-win_x86-64_cmprssptrs/161/
https://ci.eclipse.org/openj9/job/Test-extended.system-JDK8-win_x86/155/

[pshipton]

@hangshao0

[hangshao0]

@Mesbah-Alam has shorten the cacheDir to make this test pass in this PR: adoptium/openj9-systemtest#80. The cache file path is less than 260, but the mutex name is greater than 260. Will look into why the test passed when mutex name is > 260 character.

[hangshao0]

I printed out strlen(cacheFilePath) + J9SH_WIN_MUTEX_EXTRA_LEN in the following lines, we hit the edge case, it equals 260.

https://github.com/eclipse/openj9/blob/076070658cd79745aca4d6631d1a513856492698/runtime/shared_common/shrinit.cpp#L5208-L5209

[hangshao0]

In the trace points:

I see

17:26:59.878 0x27b9a00           j9prt.268      - j9shsem:createMutex creating set mutex 2, name = D__Users_jenkins_workspace_Test-extended.system-JDK8-win_x86-64_cmprssptrs_openjdk-tests_TestConfig_test_output_15494381348655_SharedClassesAPI_0_20190206-014007-SharedClassesAPI_results_javasharedresources__C290M4F1A64_DefinedLocGrpAccessJavaWL3_G35_mutex_set2
17:26:59.880 0x27b9a00           j9prt.265      < j9shsem:createMutex successfully created mutexes, exiting

The length of mutex name "D__Users_jenkins_workspace_Test-extended.system-JDK8-win_x86-64_cmprssptrs_openjdk-tests_TestConfig_test_output_15494381348655_SharedClassesAPI_0_20190206-014007-SharedClassesAPI_results_javasharedresources__C290M4F1A64_DefinedLocGrpAccessJavaWL3_G35_mutex_set2" is already 261. There are two "\"s after "javasharedresources" in the name somehow, so it has one more character than what I got in checkCacheFilePath() previously.

I tried with a longer cache name: -Xshareclasses:name=DefinedLocGrpAccessJavaWL34567890

I still see mutex creation is successful even when the mutex name has 268 characters:

17:25:41.957 0x35b9a00           j9prt.268      - j9shsem:createMutex creating set mutex 2, name = D__Users_jenkins_workspace_Test-extended.system-JDK8-win_x86-64_cmprssptrs_openjdk-tests_TestConfig_test_output_15494381348655_SharedClassesAPI_0_20190206-014007-SharedClassesAPI_results_javasharedresources__C290M4F1A64_DefinedLocGrpAccessJavaWL34567890_G35_mutex_set2
17:25:41.960 0x35b9a00           j9prt.265      < j9shsem:createMutex successfully created mutexes, exiting

I also printed out the unicodeMutexName passed to CreateMutexW, it is the same as the string in the trace point j9prt.268. So CreateMutexW() does not return NULL when mutex name is > MAX_PATH. However the documentation says "The name is limited to MAX_PATH characters".

https://docs.microsoft.com/en-us/windows/desktop/api/synchapi/nf-synchapi-createmutexw

I think we should still follow the documentation to put the restriction of MAX_PATH on the mutex name.

[keithc-ca]

Why don't we choose shorter names in general? Perhaps based on the hex of a sha1 hash code of something we expect to be unique?

[hangshao0]

The cache file path is unique. It could be shorter if we use hash code of the cache file path to be part of the mutex name.

If we decide to allow cache file path to be > MAX_PATH, another thing to do is to fix the APIs in j9shmem.c. j9shsem.c and j9shsem_deprecated.c to prepend \\?\ when necessary.

[pshipton]

We shouldn't be restricting to shorter path names than worked before. The goal was to fix the asserts, we don't want to add undue restrictions and cause a regression for a user who is using a longer path name.

[keithc-ca]

My suggestion would remove the length restriction by using names whose length does not depend on the input path length.

[hangshao0]

One thing I realize is that if we change the mutex name, we might cause issues for users who are running with an existing cache.

[keithc-ca]

Doesn't the name of the cache depend on the version (git SHA) of openj9? If so, this wouldn't affect whether an existing cache is used.

[pshipton]

The name of the cache doesn't depend on the SHA, but the VM does use the SHA to check if the JVM is different from the JVM which created the cache, and if so try to recreate it. On Windows the cache can only be deleted if is is not in use, otherwise the cache created by the older JVM continues to be used.

[hangshao0]

There is a CML option that user can disable the check for OpenJ9 sha. Even if the option is not set, we will try destroying the cache once if we detect the openj9 sha is different. However, if the deletion failed (which is possible on Windows), we will startup the shared cache ignoring the different sha.

Also when we detect the openj9 sha is different, we will acquire the mutex first then destroy the cache. If the mutex name has been changed in the newer JVM, we are not able to acquire the original mutex.

[hangshao0]

The goal was to fix the asserts, we don't want to add undue restrictions and cause a regression for a user who is using a longer path name.

If we are fixing the assert only, #4532 can be reverted.

[hangshao0]

I will create another PR to fix the assert. OMR change might required.

[pshipton]

#4532 reverted. I'll assume this issue is fixed and close it. #4351 is reopened.