You are viewing a plain text version of this content. The canonical link for it is here.

Posted to commits@kie.apache.org by "tkobayas (via GitHub)" <gi...@apache.org> on 2026/10/14 08:43:05 UTC

[I] Review ksession.dispose in unit tests [incubator-kie]

tkobayas opened a new issue, #7187:
URL: https://github.com/apache/incubator-kie/issues/7187

   Most of our unit tests don't call `ksession.dispose`. While we haven't hit OutOfMemory, it's better to check the retention. e.g., drools-model-codegen BaseModelTest creats KieContainer with UUID based ReleaseId, so ksessions are piled up...? Also Copilot review repeatedly points at it (annoying).
   
   Review it. If any issue, fix them all.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org

Re: [I] Review ksession.dispose in unit tests [incubator-kie]

Posted by "tkobayas (via GitHub)" <gi...@apache.org>.

tkobayas commented on issue #7187:
URL: https://github.com/apache/incubator-kie/issues/7187#issuecomment-6211476574

   @pibizza I think --- even if we removed them now, we couldn't keep it that way: new tests (written by people or generated by Copilot/agents) will naturally include dispose() since it's the correct API usage, and we can't ban it mechanically because some cases do need it. So "dispose is optional in tests, keep what's there" is the only rule that doesn't need policing.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org

Re: [I] Review ksession.dispose in unit tests [incubator-kie]

Posted by "tkobayas (via GitHub)" <gi...@apache.org>.

tkobayas closed issue #7187: Review ksession.dispose in unit tests
URL: https://github.com/apache/incubator-kie/issues/7187


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org

Re: [I] Review ksession.dispose in unit tests [incubator-kie]

Posted by "pibizza (via GitHub)" <gi...@apache.org>.

pibizza commented on issue #7187:
URL: https://github.com/apache/incubator-kie/issues/7187#issuecomment-6188814969

   Do we think it may be worthy removing the dispose in existing tests? it pollutes the existing codebase if it is unnecessary code.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org

Re: [I] Review ksession.dispose in unit tests [incubator-kie]

Posted by "tkobayas (via GitHub)" <gi...@apache.org>.

tkobayas commented on issue #7187:
URL: https://github.com/apache/incubator-kie/issues/7187#issuecomment-6211514618

   To suppress Copilot's suggestion, `.github/copilot-instructions.md` seems to help... I'll check and file a PR.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org

Re: [I] Review ksession.dispose in unit tests [incubator-kie]

Posted by "tkobayas (via GitHub)" <gi...@apache.org>.

tkobayas commented on issue #7187:
URL: https://github.com/apache/incubator-kie/issues/7187#issuecomment-6188429434

> `drools-model-codegen` BaseModelTest creats KieContainer with UUID based ReleaseId, so ksessions are piled up...?

No. KieContainer is not retained unless created by `newKieContainer(containerId, ...)` (ReleaseId is not relevant), so Kiecontainer, KieBase and KieSession are GC'ed. Same for ksession creation in `drools-test-coverage`.

Possible ksession leak scenarios are:
1. **Long-lived container or KieBase**: `getKieClasspathContainer()`, `newKieContainer(containerId, ...)` (cached in `KieServicesImpl`), or a KieBase held in a static / `@BeforeAll` field. Every undisposed session accumulates in `kbase.statefulSessions` for the life of the JVM (surefire fork). Incremental updates (`addPackages`, etc.) also keep updating those stale sessions.
2. **Realtime clock + a timer that is actually scheduled** (`timer`, `@expires`, etc.): `JDKTimerService` owns a `ScheduledThreadPoolExecutor`. Its core thread is non-daemon and never times out. Only `dispose()` calls `shutdownNow()`. Without it the thread leaks, and pending jobs keep the session reachable. Pseudo-clock tests are not affected.
3. **MBeans enabled**: `DroolsManagementAgent` (static) keeps the session registered until `dispose()`.

They are very rare cases, so it wouldn't cause OutOfMemory even if they pile up.

## Conclusion
We don't need to dispose ksession in unit tests. We may ignore Copilot suggestions about it.

--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@kie.apache.org
For additional commands, e-mail: commits-help@kie.apache.org