Vampire commented on PR #289: URL: https://github.com/apache/groovy-geb/pull/289#issuecomment-3357285999
Argh, damn, seems we both missed that the driver is returned there, sorry. Seems also like some tests should be added. The "fix" you do here is flaky actually. The original problem that I fixed by splitting that cache into two fields was, if configuration gets changed. And the same flakiness is in the change you added here. You still clear both caches, but you return the driver from the global cache if one was found or otherwise the one from the thread-local cache otherwise. That means that if you changed the `cacheDriverPerThread` configuration at some point, you might even have entries in both and then always return the global one, no matter what the config currently is. And even if the config would be considered (which probably cannot really be done here), you still would only get one of the two drivers back. The main question probably is, what is the use-case for returning the driver there to consider what the appropriate options are. You could consider not returning the driver from that method (having a private variant for the clear-and-quit or just doing the clear there copied. Or you could consider returning a spreading driver wrapper that contains both drivers if present and each call is fanned out to both if found. The changes here should work properly as long as the option `cacheDriverPerThread` is not changed after a driver was created, so probably in most consumers out there, but nevertheless the race condition does exist. Actually, even the `clearCacheAndQuitDriver` is flaky still even with the changes here, as it will only quit the global driver if in both, one was found. With the spreading wrapper or with copying the clear code and quit both drivers this would be "fixed". But then it also needs to be defined what actually should happen on `clearCache` and `clearCacheAndQuitDriver`. Should the latter consider the current configuration and if so, what **is** the current configuration in that static context? Should both operate on both if both are found? What should both return? These API were actually broken since long time ago when the thread-local cache was added in the case of config change. My changes now just surfaced that there is also a conceptual problem in these cases. :-/ Btw. no, these methods are not just for for testing purposes as the reporter of the current problems assumed. They are documented ways to force Geb to create a new driver on next occasion: https://groovy.apache.org/geb/manual/current/#implicit-driver-management. It is just now a matter of interpretation of the documentation how these methods should work and what they should return if anything. Maybe it would indeed be best to just not return anything from those methods. :-/ -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
