On Fri, Dec 13, 2013 at 4:02 PM, Mike Drob <[email protected]> wrote: [snip] >> If you're using Eclipse, did you know that you can delete warnings?
They show up again after each build. It's too much noise. >> Given that by making the interface "Closeable", we're in effect >> recommending that users close it, we should probably follow our own >> recommendation, so "2" is probably not a good idea, and "3" is >> probably better. I don't have time to go back and do "3", though. >> >> > What is your time frame for 3 happening? I obviously can't promise anything > for today, but can attempt to address this next week. I'm satisfied with just #4, so I'm not pushing for #3. If somebody contributes a patch to do it before 1.6 release (or maybe 1.4.5 if that gets released first), that's okay with me. >> "4" might be a good option, at least for 1.4.5-SNAPSHOT and >> 1.5.1-SNAPSHOT, so we don't convey the idea (which represents a change >> in API semantics) that you *should* close the Instance. Rather, it >> conveys the idea that it's optional, which I think is more consistent >> with those previous versions, and is suitable for the vast majority of >> use cases. >> > > Making things Closeable was an intermediate step to making them > AutoCloseable later. As you said in chat, these should have always been > Closeable, but we just never gave clients a way to do that. It caused tons > of heartache for users running from inside of web containers, and I think > the existence of the interface is a strong hint for best practices. Which is why I think #4 was a good compromise... because the close method would still be there to serve that purpose, but it wouldn't convey users *must* close it (thereby making their code incompatible with 1.4.0-1.4.4 and 1.5.0), and it solves my immediate need of having a clean build. >> All of this is completely overshadowing the real issue, though, which >> is that the close method doesn't actually prevent the resources from >> being opened again. It's a superficial fix, that doesn't really >> enforce it. Our API looks like it's stateless, with factory methods... >> but it's not actually stateless. We can close the instance, but the >> resources that were left open aren't isolated to the instance... they >> are used inside the Connector and below. Closing the instance may free >> up resources, but it doesn't stop new ones from being opened again >> inside the connector and below. The problem is that the "Instance" >> object does not fully represent the resources used inside client code, >> so closing it is semantically unintuitive, incorrect, and functionally >> broken if not used in a very specific way. > > > Can you elaborate on this? Closing an instance should close all of the > underlying objects as well (and I was under the impression that it did). [snip] None of the underlying objects are Closeable (either with that interface, or even semantically). Take Connector, for example. How do you define the semantics for "closing" a factory? It's not impossible... but it's a bit unintuitive... and very different semantics from prior releases. -- Christopher L Tubbs II http://gravatar.com/ctubbsii
