On Tue, 19 May 2026 06:00:51 GMT, Alan Bateman <[email protected]> wrote:
>>> I checked the API docs in the latest commit and I have more comments on the >>> API changes: >> >> I've been looking at using AttachNotSupportedException for some of the >> problems that can happen along the way. >> That can include an early check that finds an unusable revival cache dir in >> the Map env, but also bad core file, or wrong platform (for dirs for library >> lookup, failure is not fatal, but badly specified revival cache dir is >> fatal). >> >> AttachNotSupportedException is already used for some "bad vmid" problems, so >> although "not supported" is not the phrase I'd choose, it can be the right >> Exception to throw for various problems. >> >> When we get as far as execuring jcmd, IOException seems to be the way to say >> it went wrong. >> >> Will check into all these points.... > >> AttachNotSupportedException is already used for some "bad vmid" problems, so >> although "not supported" is not the phrase I'd choose, it can be the right >> Exception to throw for various problems. > > The API iterates over the installed providers until it finds one that can > attach to the target VM (or all fail). It would be a valid choice for this > JEP to add another attach provider implementation rather than add to the > existing attach provider. > > AttachProvider.attachVirtualMachine is specified to throw > AttachNotSupportedException "If the identifier cannot be parsed, or it > corresponds to to a Java virtual machine that does not exist, or it > corresponds to a Java virtual machine which this provider cannot attach". > With main line, if you invoke attach with a file path as the id then it will > throw AttachNotSupportedException already. Thanks @AlanBateman > 1. It's not clear right now unless you click on "attachVirtualMachine" to see > if it calls the 1-arg or 2-arg attachVirtualMachine method. Yes, done, it can be clearer that the 2-arg VirtualMachine.attach calls the provider method with the same parameters. > 2. Can you explain again why the 1-arg attach can't be specified to be > equivalent to the 2-arg attach with an empty map? I'm sure this will come up > in the CSR discussion. The API docs allow the env to be empty and the HotSpot > provider should be able to distinguish a process ID from a file path. This was to avoid a surprise behaviour change at the API level. Changing the existing attach method that only takes a PID, to possibly accept a filename, introduces a possible problem where they expect to attach to PID 123 but a file "123" exists. A new method is free to prefer a file if it exists, without a conflict. If they were equivalent when the map is empty: then as jcmd is not always going to be given arguments that populate the Map, it will not try to attach to a core (unless there's a behaviour change in the existing method to prefer a core). > 3 "env" is specified to as "... provider-specific settings to configure the > attach". I think this will need to be worded differently... "configure the attach" maybe can word it better. > 4. It doesn't seem possible for an attach provider to throw if env parameter > does not contain required properties, contains an invalid combination of > properties, or a bad value. I realize you don't want to eagerly validates the > values of libDirs and revivalCachePath but I think the provider interface has > to allow for this, maybe a different provider implementation, maybe a future > enhancement to this feature. The summary is that I think the 2-arg attach and > SPI attachVirtualMachine method needs to specify IllegalArgumentException. I was looking at throwing earlier if it finds an unusable revival cache dir in the Map env, but also bad core file, or wrong platform. Was trying AttachNotSupported but yes I can happily add IllegalArgumentException as that is more appropriate. > 5. The IOException that the 2-arg attachVirtualMachine can throw is not > specified. OK yes done. Yes it appears in the doc but doesn't have the same text as the others. > 6. The default implementation of 2-arg attachVirtualMachine throws UOE but > that isn't currently specified (no @throws UOE. I realize we discussed in > other comments here about invoking the 1-arg attachVirtualMachine when the id > parses as an integer and the map is empty. Looking it again, I think the > least surprising default is to just invoke the 1-arg attachVirtualMachine if > the map is empty (no checking of id). Throw AttachNotSupportedException if > not empty. I realize UOE is tempting, and I may have suggested it at one > point, but this leaks into the API and it will hard to explain UOE vs. > AttachNotSupportedException. I'm trying out the default implementation of AttachProvider.attachVirtualMachine(String id, Map<String, ?> env) simply calling attachVirtualMachine(String id) if the Map is empty, I think that should be fine. Seems it should throw if Map is not empty. AttachNotSupportedExeption would be fine. The base AttachProvider.attachVirtualMachine(String id, Map<String, ?> env) is commonly going to be overridden by HotSpotAttachProvider. jcmd may not be given options that populate the Map env, but may still expect to open a core file. So if HotSpotAttachProvider delegates to attachVirtualMachine(vmid) on getting an empty Map env, we can't open a core. This is similar to (2) in keeping core files only openable by the 2-arg attach method (id, env). Yes I was avoiding writing a new AttachProvider. It would mean the precedence of filename and PID might be set by iteration order of the Providers. (code update to follow) ------------- PR Comment: https://git.openjdk.org/jdk/pull/31011#issuecomment-4487349351
