----- Original message ----- > > @Patricia - I understand your interpretation, and I think I’ve misled > you by including the words “concrete, observable” in my question. I’m > not talking about concurrency bugs here. I’m perfectly OK with making > changes based on analysis that shows a possible race condition. > However, I spent most of yesterday reading the Java Language > Specification and I’m now more convinced than ever that statements like > “we’re using final variables, therefore all our code has to change” > (paraphrasing) are no substitute for reasoned analysis.
You don't agree that final field usage should comply with the JMM? Please also read the Java Memory Model. Basically, I’m > asserting the very common professional view that changes to existing > code should be traceable to a requirement or a problem. Further, those > changes should be carefully considered and designed to minimize the > chance of breaking anything else. I don’t find the argument that “I > fixed a concurrency problem, so it’s expected to have 35 other failures” > convincing. From the outside, that looks a lot like introducing > failures in an attempt to fix a poorly understood or poorly stated > problem. You're misinterpreting my development effort, I haven't yet fixed ServiceDiscoveryManager, but I have fixed other classes, until only a few commits ago ServiceDiscoveryManager was not failing (at least not often). Although ServiceDiscoveryManager hasn't changed since then, it is now causing multiple test failures. So while the other classes that use ExecutorService aren't failing, ServiceDiscoveryManager is failing precisely because we're using TaskManager's fundamentally broken runAfter method. We have previously focused on the problem that runAfter was designed to address and found this was not a good solution to the original problem. > > I teach programming. I see this all the time. When people make changes > based on what they “think” “might” be happening, it’s always a disaster. You're assuming FindBugs, JMM non-compliance and common issues described by Concurrency in Practise, when clearly and easily identified fall into the same category as someone who's learning to code for the first time? You're advocating a completely experimental driven approach, like someone who's learning to program experiences. It is very good approach for learning... Do you not teach your students how to safely use final fields? > > @Peter - You’re asking the wrong question. The right question is > whether we should have a review-then-commit policy for existing code > that traces a well-defined (and ideally limited) package of changes to a > JIRA issue, This would prevent a move from experimental to include a more theoretical development approach. Test cases are still required to pass with a theoretical approach. and includes the community in decisions around how to fix > the problem that is stated. And when a simple discussion about a simple issue veers off topic with heated arguments, what then? You propose I stop committing to skunk, until each change is discussed at length on the list? But we can't even agree on final field usage? We need to make a decision whether to include theoretical driven development that includes compliance to standards like the JMM. Code has been committed to skunk, for people to review, in public, they might misinterpret my suggestions on the mail list, but they will have a better understanding after reading the code, I welcome their participation. I am open to other solutions, but I don't like the suggestion that non compliant code is preferred until a test case that proves failure can be found. What happens when the test case only fails on one platform, but is not easily repeatable? What if the fix just changes timing to mask the original problem? Testing doesn't prove the absence of errors? Reducing errors further requires additonal tools like FindBugs and public open peer review. Trunk development was halted because of unrelated test failures. If you try to fix those test failures it will cause a cascading effect of additional new test failures. I am still fixing those bugs in qa-refactor. If the outcome of this discussion is to exclude theoretical driven development (FindBugs, JMM compliance), then the only option will be to revert trunk and exclude code that causes unrelated test failures. The java platform is also undergoing development, this too will cause timing changes resulting in test failures. What do we do then when we can't fix them because each fix causes other unrelated test failures? On that note, Java 7 support is not well tested, 2.2 has only been tested on Linux x64, no results for the jtreg tests have been posted and I have previously seen discovery event related test failures in the 2.2 branch. The qa-refactor branch is relatively stable on Linux x64. The test failures we're discussing occur on Windows x64, perhaps they also occur with 2.2 on windows, should we test 2.2 on windows too? If 2.2 fails on windows that gives us a provable test case doesn't it? The test failures are then proven to be only indirectly related to changes in qa-refactor? Regards, Peter. And I will suggest pre-emptively that a > JIRA issue that says “concurrency problems exist” is not specific enough > to drive development. > > Greg. > > On Jan 4, 2014, at 4:59 AM, Peter Firmstone <j...@zeus.net.au> wrote: > > > Please provide your thoughts on the following: > > > > How do we develop code for River? > > > > Do we only use experimental based development, or do we also allow > > theoretical development also? Do we only fix bugs that can be > > demonstrated with a test case, or do we fix bugs identified by > > FindBugs and manual code auditing as well? > > > > Should we allow theoretical development based on standards like the > > Java Memory Model with visual auditing and static analysis with > > FindBugs, or should we prohibit fixing bugs that don't include a test > > case demonstrating the failure? > > > > Regards, > > > > Peter. > > > > > > On 4/01/2014 6:58 PM, Patricia Shanahan wrote: > > > Just before Christmas, you were discussing whether to fix > > > concurrency problems based on theoretical analysis, or to only fix > > > those problems for which there is experimental evidence. > > > > > > I believe the PMC will be at cross-purposes until you resolve that > > > issue, and strongly advise discussing and voting on it. > > > > > > This is an example of a question whose answer would be obvious and > > > non-controversial if you had agreement, either way, on that general > > > issue. "When do you claim that this happens? And what currently > > > happens now that is unacceptable? What is the concrete, observable > > > problem that you’re trying to solve, that justifies introducing > > > failures that require further work?" is a valid, and important, set > > > of questions if you are only going to fix concurrency bugs for which > > > there is experimental evidence. It is irrelevant if you are going to > > > fix concurrency bugs based on theoretical analysis. > > > > > > Patricia > > > > > > On 1/3/2014 10:14 PM, Greg Trasuk wrote: > > > > > > > > On Jan 4, 2014, at 12:52 AM, Peter Firmstone <j...@zeus.net.au> > > > > wrote: > > > > > > > > > On 4/01/2014 3:18 PM, Greg Trasuk wrote: > > > > > > I’ll also point out Patricia’s recent statement that > > > > > > TaskManager should be reasonably efficient for small task > > > > > > queues, but less efficient for larger task queues. We don’t > > > > > > have solid evidence that the task queues ever get large. > > > > > > Hence, the assertion that “TaskManager doesn’t scale” is > > > > > > meaningless. > > > > > > > > > > No, it's not about scalability, it's about the window of time > > > > > when a task is removed from the queue in TaskManager for > > > > > execution but fails and needs to be retried later. > > > > > Task.runAfter doesn't contain the task that "should have > > > > > executed" so dependant tasks proceed before their depenencies. > > > > > > > > > > This code comment from ServiceDiscoveryManager might help: > > > > > > > > > > /** This task class, when executed, first registers to receive > > > > > * ServiceEvents from the given ServiceRegistrar. If the > > > > > registration * process succeeds (no RemoteExceptions), it then > > > > > executes the * LookupTask to query the given ServiceRegistrar > > > > > for a "snapshot" * of its current state with respect to > > > > > services that match the * given template. > > > > > * > > > > > * Note that the order of execution of the two tasks is > > > > > important. * That is, the LookupTask must be executed only > > > > > after registration * for events has completed. This is because > > > > > when an entity registers * with the event mechanism of a > > > > > ServiceRegistrar, the entity will * only receive notification > > > > > of events that occur "in the future", * after the registration > > > > > is made. The entity will not receive events * about changes to > > > > > the state of the ServiceRegistrar that may have * occurred > > > > > before or during the registration process. * > > > > > * Thus, if the order of these tasks were reversed and the > > > > > LookupTask * were to be executed prior to the > > > > > RegisterListenerTask, then the * possibility exists for the > > > > > occurrence of a change in the * ServiceRegistrar's state > > > > > between the time the LookupTask retrieves * a snapshot of that > > > > > state, and the time the event registration * process has > > > > > completed, resulting in an incorrect view of the * current > > > > > state of the ServiceRegistrar. > > > > > > > > > > > > > When do you claim that this happens? And what currently happens > > > > now that is unacceptable? What is the concrete, observable > > > > problem that you’re trying to solve, that justifies introducing > > > > failures that require further work? > > > > > > > > >
