InMemPagedTRS handles concurrency. by Jad-el-khoury · Pull Request #855 · eclipse-lyo/lyo

Jad-el-khoury · 2025-12-05T08:20:46Z

InMemPagedTRS handles concurrency.

Checklist

This PR adds an entry to the CHANGELOG. See https://keepachangelog.com/en/1.0.0/ for instructions. Minor edits are exempt.
This PR was tested on at least one Lyo OSLC server (comment @oslc-bot /test-all if not sure) or adds unit/integration tests.
This PR does NOT break the API
Lint checks pass (run mvn package org.openrewrite.maven:rewrite-maven-plugin:run spotless:apply -DskipTests -P'!enforcer' if not, commit & push)

Jad-el-khoury · 2025-12-05T08:21:17Z

@berezovskyi See it working with oslc-op/refimpl#473 on the rm-server

Copilot

Pull request overview

This PR adds concurrency support to the InMemPagedTRS implementation by transitioning from List-based to ConcurrentHashMap-based storage for base resources and change logs, and using CopyOnWriteArrayList for thread-safe collections in Base and ChangeLog classes.

Key changes:

Converted InmemPagedTrs internal storage from List to ConcurrentHashMap with URI-based indexing
Added thread-safe collections (CopyOnWriteArrayList) to Base and ChangeLog classes
Synchronized critical methods (onHistoryData, initBase, findOrCreateBase, etc.) to prevent race conditions during concurrent updates
Added new navigation methods to the PagedTrs interface (getBaseResource(URI), getBaseFirst(), getNext(), getChangeLog(URI), getPrevious())
Added comprehensive concurrency test (InmemPagedTrsThreadSafetyTest) to verify thread-safety under concurrent access

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java	Refactored to use ConcurrentHashMap for storage, added synchronized methods, implemented URI-based lookups
trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/PagedTrs.java	Added new interface methods for URI-based lookups and navigation
trs/server/src/test/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrsThreadSafetyTest.java	New concurrency test validating thread-safe behavior under concurrent updates
trs/server/src/test/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrsTest.java	Updated tests to use new URI-based API methods
core/oslc-trs/src/main/java/org/eclipse/lyo/core/trs/Base.java	Changed members list to CopyOnWriteArrayList for thread-safety, updated setter to be defensive
core/oslc-trs/src/main/java/org/eclipse/lyo/core/trs/ChangeLog.java	Changed change list to CopyOnWriteArrayList for thread-safety, updated setter to be defensive, fixed spacing in annotation
trs/client/trs-client/src/main/java/org/eclipse/lyo/trs/client/util/ProviderUtil.java	Wrapped subList() results with ArrayList to prevent issues with list views
trs/server/pom.xml	Added junit-vintage-engine dependency for test compatibility
CHANGELOG.md	Added entry documenting concurrency support

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-05T11:11:15Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java

+        if (TRSConstants.RDF_NIL.equals(base.getNextPage().getNextPage().toString())) {
+            return null;
+        }
+        return getBaseResource(base.getNextPage().getNextPage());


This method will throw a NullPointerException if base.getNextPage() or base.getNextPage().getNextPage() returns null. Add null checks before calling toString() and accessing nested properties to handle these edge cases safely.

Suggested change

if (TRSConstants.RDF_NIL.equals(base.getNextPage().getNextPage().toString())) {

return null;

}

return getBaseResource(base.getNextPage().getNextPage());

if (base == null) {

return null;

}

Page nextPage = base.getNextPage();

if (nextPage == null) {

return null;

}

URI nextNextPage = nextPage.getNextPage();

if (nextNextPage == null) {

return null;

}

if (TRSConstants.RDF_NIL.equals(nextNextPage.toString())) {

return null;

}

return getBaseResource(nextNextPage);

Copilot · 2025-12-05T11:11:15Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java

-    private Base getLastBaseResource() {
-        return this.baseResources.get(this.baseResources.size() - 1);
+    private URI lastBasePageUri() {
+        int pageId = this.baseResources.size();


The lastBasePageUri() method can fail when baseResources.size() is 0, as basePageUriForPage(0) will throw an IllegalArgumentException (page id must be >= 1). This method is called by getLastBaseResource() which is used in synchronized methods. Add a check to handle the empty base resources case.

Suggested change

int pageId = this.baseResources.size();

int pageId = this.baseResources.size();

if (pageId == 0) {

return null;

}

Copilot · 2025-12-05T11:11:16Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java

+        		.path(this.baseRelativePath)
+        		.path(String.valueOf(pageId))


Use consistent indentation: lines 382-383 use tab characters while the surrounding code uses spaces. This affects code readability and consistency.

Suggested change

.path(this.baseRelativePath)

.path(String.valueOf(pageId))

.path(this.baseRelativePath)

.path(String.valueOf(pageId))

Copilot · 2025-12-05T11:11:16Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/PagedTrs.java


+    /**
+     * Get the previous ChangeLog of the given changeLog.
+     *


The @param tag is missing for the changeLog parameter. The JavaDoc should document this parameter: @param changeLog the current ChangeLog page

Suggested change

*

*

* @param changeLog the current ChangeLog page

Copilot · 2025-12-05T11:11:16Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java

-        final URI uri = getUriBuilder().path(this.baseRelativePath).build();
-        return uri;
+    private URI nextBasePageUri() {
+    	int pageId = this.baseResources.size() + 1;


Use consistent indentation: this line uses a tab character while the surrounding code uses spaces. This affects code readability and consistency.

Suggested change

int pageId = this.baseResources.size() + 1;

int pageId = this.baseResources.size() + 1;

Copilot · 2025-12-05T11:11:16Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java

+    	//There is always an entry with the "nil" changeLog page.
+    	return changelogUriForPage(this.changelogResources.size());


Use consistent indentation: this line uses a tab character while the surrounding code uses spaces. This affects code readability and consistency.

Suggested change

//There is always an entry with the "nil" changeLog page.

return changelogUriForPage(this.changelogResources.size());

//There is always an entry with the "nil" changeLog page.

return changelogUriForPage(this.changelogResources.size());

Copilot · 2025-12-05T11:11:17Z

trs/server/src/test/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrsThreadSafetyTest.java

+        }
+
+        exec.shutdown();
+        exec.awaitTermination(10, TimeUnit.SECONDS);


The test doesn't verify that the executor service completed successfully. If awaitTermination returns false (meaning timeout occurred), the test should fail. Consider checking the return value: Assert.assertTrue("Executor did not complete in time", exec.awaitTermination(10, TimeUnit.SECONDS));

Copilot · 2025-12-05T11:11:17Z

trs/server/src/main/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrs.java

-    private int pageIdToListIdx(final Integer pageId) {
-        return pageId - 1;
+    @Override
+    public Base getBaseFirst() {


The getBaseFirst() method will throw a NoSuchElementException if baseResources is empty. This can occur if the TRS is initialized with an empty collection or before any base resources are added. Consider adding a null check or isEmpty() check to return null when there are no base resources.

Suggested change

public Base getBaseFirst() {

public Base getBaseFirst() {

if (baseResources.isEmpty()) {

return null;

}

Copilot · 2025-12-05T11:11:17Z

trs/server/src/test/java/org/eclipse/lyo/oslc4j/trs/server/InmemPagedTrsThreadSafetyTest.java

+                            : (totalEvents % changelogPageLimit == 0
+                                ? changelogPageLimit
+                                : totalEvents % changelogPageLimit);


Test is always false.

Suggested change

: (totalEvents % changelogPageLimit == 0

? changelogPageLimit

: totalEvents % changelogPageLimit);

: totalEvents % changelogPageLimit;

Copilot · 2025-12-05T11:11:18Z

core/oslc-trs/src/main/java/org/eclipse/lyo/core/trs/Base.java

+    @OslcDescription("A member Resource of the Resource Set.")
+    @OslcPropertyDefinition(RDFS_MEMBER)
+    @OslcTitle("Member")
+    public List<URI> getMembers() {


getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.
getMembers exposes the internal representation stored in field members. The value may be modified after this call to getMembers.

berezovskyi

Will need to have a closer look

berezovskyi · 2025-12-05T11:06:28Z

core/oslc-trs/src/main/java/org/eclipse/lyo/core/trs/Base.java

+        if (members == null) {
+            throw new IllegalArgumentException("Members list must not be null");
+        }
+        this.members.clear();


This does not seem thread safe. As no lock is used, there could be a case where a reader would observe an empty list while this update is going on. Can we not just introduce an api to add an item? Otherwise it's best to mark this method synchronised.

berezovskyi · 2025-12-05T11:07:41Z

core/oslc-trs/src/main/java/org/eclipse/lyo/core/trs/Base.java

+    @OslcDescription("A member Resource of the Resource Set.")
+    @OslcPropertyDefinition(RDFS_MEMBER)
+    @OslcTitle("Member")
+    public List<URI> getMembers() {


Can we update this signature to return a readonly list? To stop the pattern of getting a list and then modifying it by hand.

berezovskyi · 2025-12-05T11:08:52Z

trs/client/trs-client/src/main/java/org/eclipse/lyo/trs/client/util/ProviderUtil.java


-        firstChangelogEvents = firstChangelogEvents.subList(indexOfSync + 1,
-                firstChangelogEvents.size());
+        firstChangelogEvents = new ArrayList<>(firstChangelogEvents.subList(indexOfSync + 1,


I don’t like this, it's really prone to misuse. I can see how a user may forget to do this and get occasional concurrency issues.

I also think that copy on write list AND a new arraylist are quite wasteful when used together?

berezovskyi · 2025-12-13T17:02:50Z

@Jad-el-khoury, see my feedback inside but if you are happy to merge as is, let's do it. We can convert some comments into new issues.

InMemPagedTRS handles concurrency.

205e765

Jad-el-khoury requested a review from berezovskyi December 5, 2025 08:20

bug fix in trs-client tests.

117ac80

berezovskyi requested a review from Copilot December 5, 2025 11:02

Copilot started reviewing on behalf of berezovskyi December 5, 2025 11:02 View session

Copilot finished reviewing on behalf of berezovskyi December 5, 2025 11:05

Copilot AI reviewed Dec 5, 2025

View reviewed changes

berezovskyi reviewed Dec 5, 2025

View reviewed changes

Merge branch 'master' into trs_concurrency

8c96f55

berezovskyi approved these changes Dec 13, 2025

View reviewed changes

berezovskyi merged commit ba6a796 into master Dec 13, 2025
10 checks passed

berezovskyi added this to the 7.0 milestone Dec 13, 2025

	int pageId = this.baseResources.size() + 1;
	int pageId = this.baseResources.size() + 1;

		//There is always an entry with the "nil" changeLog page.
		return changelogUriForPage(this.changelogResources.size());

Comments

Conversation

Jad-el-khoury commented Dec 5, 2025

Checklist

Uh oh!

Jad-el-khoury commented Dec 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

berezovskyi left a comment

Choose a reason for hiding this comment

Uh oh!

berezovskyi Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

berezovskyi Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

berezovskyi Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

berezovskyi Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

berezovskyi commented Dec 13, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants