Close streams fetched from data-providers

[heruan]

This is an attempt to fix #18551 for the DataCommunicator API by handling the streams fetched from data-providers inside try-with-resources blocks.

Unclosed streams might still leak from the DataView API, that should be fixed in each component using that API.

[github-actions]

Test Results

1 092 files  ± 0  1 092 suites  ±0   1h 25m 14s ⏱️ + 2m 40s
6 828 tests + 7  6 782 ✅ + 7  46 💤 ±0  0 ❌ ±0 
7 157 runs   - 13  7 099 ✅  - 13  58 💤 ±0  0 ❌ ±0 

Results for commit c513d13. ± Comparison against base commit dc68b28.

♻️ This comment has been updated with latest results.

[mcollovati]

The change looks good to me 👍
Two points we should probably consider:

• Updating DataView javadoc to suggest the user to use the returned stream in combination with try-with-resource to prevent leaking resources (as noted in the PR description). Possibly adding a simple code example.
• Make HierarchicalDataCommunicator work consistently, by applying try-with-resource where possible also in HierarchyMapper (e.g. getFlatChildrenStream()) and HierarchicalCommunicationController (e.g. activate method).

[heruan]

• Updating DataView javadoc to suggest the user to use the returned stream in combination with try-with-resource to prevent leaking resources (as noted in the PR description). Possibly adding a simple code example.

Updated the JavaDoc with the recommendation and code example.

• Make HierarchicalDataCommunicator work consistently, by applying try-with-resource where possible also in HierarchyMapper (e.g. getFlatChildrenStream()) and HierarchicalCommunicationController (e.g. activate method).

Closing streams also for hierarchical communicators and mapper now. I'm just having a hard time understanding how unit tests work for the hierarchical API, I added one covering HierarchicalCommunicationController::activate and HierarchyMapper::getChildrenStream but I couldn't hit HierarchyMapper::getFlatChildrenStream.

[mcollovati]

@heruan great work!
I think we can test HierarchyMapper::getFlatChildrenStream indirectly in HierarchyMapperWithDataTest by calling HierarchyMapper.fetchHierarchyItems()

Something like

    @Test
    public void fetchHierarchyItems_streamIsClosed() {
        AtomicBoolean streamIsClosed = new AtomicBoolean();
        mapper = new HierarchyMapper<>(new TreeDataProvider<>(data) {
            @Override
            public Stream<Node> fetchChildren(HierarchicalQuery<Node, SerializablePredicate<Node>> query) {
                return super.fetchChildren(query)
                        .onClose(() -> streamIsClosed.set(true));
            }
        });
        Node rootNode = testData.get(0);
        mapper.expand(rootNode);
        mapper.fetchHierarchyItems(rootNode, Range.between(0, 10)).count();

        Assert.assertTrue(streamIsClosed.get());
    }

[heruan]

@heruan great work! I think we can test HierarchyMapper::getFlatChildrenStream indirectly in HierarchyMapperWithDataTest by calling HierarchyMapper.fetchHierarchyItems()

Oh there it is, thanks! I added two tests to HierarchyMapperWithDataTest for consistency, one for getFlatChildrenStream and one for getChildrenStream.

[sonarcloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud