Conversation
3 tasks
|
@reckart are we merging this fix, we are waiting for this fix to merge so that we can proceed with refactoring/442-Upgrade-to-UIMAv3 |
Member
Author
|
There is no fix yet - I need to find time to investigate it further because changes in this PR do not yet resolve the problem. The alternative would be a workaround. I think it might be sufficient to create a dummy CAS before the failing tests accesses the JCas classes. |
Member
Author
|
The work in this branch so far is unable to fix the problem. The recommended workaround is to create a CAS object before working with any JCas classes. |
reckart
added a commit
that referenced
this pull request
Sep 23, 2024
Issue #235: Update dependencies
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #234 where loading built-in JCas cover classes (notably Annotation) before any CAS exists can corrupt type system/JCas type-id bookkeeping and prevent feature callsite updates, leading to failures when later creating/using a CAS.
Changes:
- Adds a regression test that reproduces the problem in a fresh forked JVM and asserts the subprocess exits cleanly.
- Defers committing
TypeSystemImpl.staticTsifrom class initialization time to a new lazycommittedStaticTsi()accessor to avoid recursive class-init storms. - Updates
TypeSystemConstantsto obtain built-in feature adjusted offsets viaTypeSystemImpl.committedStaticTsi().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
uimaj-core/src/test/java/org/apache/uima/jcas/impl/LoadingBuiltinAnnotationBeforeCasTest.java |
New regression test that forks a fresh JVM to reproduce/guard against issue #234. |
uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemImpl.java |
Switches static built-in type system commit to lazy-on-demand to prevent recursive initialization issues. |
uimaj-core/src/main/java/org/apache/uima/cas/impl/TypeSystemConstants.java |
Routes built-in feature adjusted-offset constants through the new lazy-commit accessor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
reckart
force-pushed
the
bugfix/234-Using-builtin-annotation-classes-before-creating-a-CAS-can-break-type-system-management
branch
2 times, most recently
from
aa308af to
df242a5
Compare
…n break type system management - Try fixing the case if a JCCI was created with a bad type ID because the type ID had not been set yet
…n break type system management - Revert bad fix - Add proper reproducer test
…n break type system management - Defer `TypeSystemImpl.staticTsi.commit()` out of `<clinit>` into a new lazy `committedStaticTsi()` accessor with double-checked-locking on a volatile flag, breaking the recursive cover-class init storm that caused the issue - Route `TypeSystemConstants` builtin-feature `adjOffset` constants through `TypeSystemImpl.committedStaticTsi()` so they remain valid now that `staticTsi` is no longer committed eagerly
…n break type system management - Drop dead `if (staticTsi == null)` guard in `TypeSystemImpl.createCallSiteForBuiltIn` and route the offset lookup through the cached `committedStaticTsi` field (read directly, not via the accessor, to avoid forcing a commit from inside a built-in cover class's `<clinit>` and reintroducing the issue-#234 init storm); fall through to a default callsite when no commit has happened yet - Replace OS-detection-based `java`/`java.exe` path construction in `LoadingBuiltinAnnotationBeforeCasTest` with `ProcessHandle.current().info().command()` plus a `${java.home}/bin/java` fallback (CreateProcess auto-resolves `.exe` on Windows) - Drain the forked JVM's output on a daemon thread so the 60s `waitFor` timeout actually fires when the subprocess deadlocks instead of being blocked indefinitely by a synchronous `transferTo`
…n break type system management - Fix flaky test FSClassRegistryTest by asserting on specific classloader registration instead of global cache size - Add test support method isClToType2JCasRegistered() to FSClassRegistry to check classloader registration state - Remove dependency on process-wide static weak map size which is non-deterministic due to asynchronous GC reaping
reckart
force-pushed
the
bugfix/234-Using-builtin-annotation-classes-before-creating-a-CAS-can-break-type-system-management
branch
from
e6506db to
cbf21b7
Compare
reckart
deleted the
bugfix/234-Using-builtin-annotation-classes-before-creating-a-CAS-can-break-type-system-management
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What's in the PR
How to test manually
TimeMlAnnotateTestcaughAutomatic testing
Documentation
Organizational
Only dependencies under approved licenses are allowed. LICENSE and NOTICE files in the respective modules where dependencies have been added as well as in the project root have been updated.