Add MustCall annotations to resource-owning JDBC, MIDI, and audio interfaces#254
Add MustCall annotations to resource-owning JDBC, MIDI, and audio interfaces#254parameshn wants to merge 8 commits intotypetools:masterfrom
Conversation
This ensures that close() obligations are enforced consistently and propagate to all subinterfaces such as Mixer, SourceDataLine, and TargetDataLine, without requiring redundant annotations on each subtype. Audio Lines own native system resources; failing to call close() leaks OS-level audio handles. Annotating Line enforces this lifecycle and correctly propagates to all subtypes.
Closing a MidiDevice: -releases native MIDI resources -stops internal threads -automatically closes all associated transmitters and receivers Indicates that the application has finished using the receiver, and that limited resources it requires may be released or made available.
Closing a MidiDevice: -releases native MIDI resources -stops internal threads -automatically closes all associated transmitters and receivers Indicates that the application has finished using the receiver, and that limited resources it requires may be released or made available.
All ResultSet and RowSet implementations, both connected and disconnected, should be explicitly closed. Connected types release live database resources such as connections and cursors, while disconnected types still hold internal resources that should not be left to garbage collection, especially in long running applications.
| @AnnotatedFor({"mustcall"}) | ||
| @InheritableMustCall("close") |
There was a problem hiding this comment.
Based on the Javadoc, I think we may need @NotOwning annotations on methods like MidiSystem#getReceiver and MidiSystem#getTransmitter, to indicate that the MidiDevice returned by those APIs does not need to be explicitly closed. Do you agree?
There was a problem hiding this comment.
From my understanding, when MidiSystem.getReceiver() / getTransmitter() are used, the underlying MidiDevice is opened implicitly by the system and closed implicitly when the returned Receiver / Transmitter is closed (assuming no other Receivers/Transmitters remain open). As a result, the application does not own the MidiDevice; its responsibility is only to close the returned Receiver / Transmitter. Since the MidiDevice is not exposed by these APIs, there isn’t really a place to annotate it with @NotOwning at the API level.
// Requests a Receiver from the system.
// Implicitly opens the underlying MidiDevice
Receiver receiver = MidiSystem.getReceiver();
// Closes the Receiver.
// Implicitly closes the MidiDevice (if this was the last open one)
receiver.close();
There was a problem hiding this comment.
I think I misunderstood what was going on here. We don't need @NotOwning annotations on these APIs.
| * @since 1.3 | ||
| */ | ||
| @AnnotatedFor({"mustcall"}) | ||
| @InheritableMustCall("close") |
There was a problem hiding this comment.
Is a Line object automatically open at the time it is constructed, is an explicit open() call required? If it's the latter, we might need to treat Line like Socket and add a @CreatesMustCallFor annotation somewhere. This might be tricky since Line is an interface.
There was a problem hiding this comment.
It’s the latter: a Line is not automatically open when obtained, and an explicit open() call is required.
Given that Line is an interface with multiple implementations and open(...) variants, annotating Line itself seems likely to be too coarse and incomplete. I think a similar argument applies to MidiDevice as well, since it is also an interface and can be opened both explicitly and implicitly. I’d appreciate your thoughts on whether there’s a better way to model this.
There was a problem hiding this comment.
This is a bit tricky. I think ideally, we'd annotate any relevant methods that create an unopened Line as @MustCall({}), and then annotate any open methods that actually acquire the resource as @CreatesMustCallFor (like here). Does this seem infeasible?
There was a problem hiding this comment.
I think we need something like this for this instance
if method overrides another method:
if overridden method has @InheritableCreatesMustCallFor:
treat current method as if it has @CreatesMustCallFor
custom one maybe?
There was a problem hiding this comment.
@msridhar If that approach seems reasonable, I’m happy to prototype it and share a draft PR for feedback before applying it to Line and MidiDevice.
There was a problem hiding this comment.
Hi
just checking in on this discussion.
There was a problem hiding this comment.
Sorry @parameshn I've been very busy, hoping to get back to this next week
There was a problem hiding this comment.
No worries, thanks for the update! Looking forward to your thoughts.
|
WPI tests are failing for JDKs 11 and 17 only (but not 21 and 25) because of changes upstream; the CF itself only runs the WPI tests on the latest LTS JDK now, and they're not supported for older JDKs. The particular problem is that the build scripts used by the target projects use |
Statement owns independent JDBC resources and must be explicitly closed. Relying on Connection.close() is unsafe (e.g., with connection pooling) and not clearly guaranteed by the JDBC specification. Annotating Statement enables sound resource-leak detection and reflects recommended JDBC usage.
|
@parameshn sorry for being slow to reply here. I wonder if we should pull out the SQL changes into their own PR? Those look fine / uncontroversial to me, whereas the audio stuff is trickier. |
|
No problem at all 🙂 Yes, I’ve split the SQL changes into a separate PR so they can be reviewed independently. I also think we can proceed with |
3 participants
Add MustCall annotations to core JDBC, MIDI, and audio interfaces that own native or external resources.
These annotations model required close() obligations and propagate correctly to all subinterfaces, enabling the MustCall Checker to detect resource leaks.
Related to typetools/checker-framework#6354.