Community
Participate
Working Groups
The process EDC uses for TCF peer discovery, selection, and connection needs work. To start with here is an e-mail thread describing some shortcomings: Hi John, Nope, I didn't do the remotePeers stuff -- Ken did that at the end of March as part of removing UI from EDC core. My changes last month were to allow the final launch sequence to document how a peer may be provided by a subclass and also to flag whether it is locally allocated (#isLocallyAllocatedPeer, for cases where a peer is not discoverable). But I agree with your points -- we do need to clean up how the peers are detected and used in the launch, since it was organically derived from our use cases in Carbide. It's a little fuzzy now, and not helped much by my changes. :) The reason I was bumbling around in there was I found that ITCFAgentLauncher didn't work for my purposes as expected. It has a possibly invalid assumption that one can implement #getServiceNames(), since this is called before you've launched the agent. So all our implements hardcode this information in the implementation. In my case I needed to make an IPeer for a custom IChannel on a local port that talking to a device-side TCF agent. It doesn't involve launching an agent, and is not available through the Locator service, and we cannot detect its services until we actually connect. We had never done this kind of thing before, and it didn't quite fit into the local vs. remote division. But maybe this discussion should go into a bug for some future changes in 8.0. -- Ed -----Original Message----- From: ext John Cortell [mailto:rat042@freescale.com] Sent: Tuesday, June 22, 2010 9:29 AM To: Swartz Ed (Nokia-D/Austin) Subject: AbstractFinalLaunchSequence.findPeer(RequestMonitor) Ed, I believe you add the logic to delegate to the AbstractFinalLaunchSequence subclass chose or override what peer to use in the launch. But why only when using remote peers? What if there were three local peers that matched the criteria set by the subclass (the contents of AbstractFinalLaunchSequence.peerAttributes)? Also, an agent-launcher is technically not only for launching local peers. An agent-launcher might not actually launch an agent but rather give instructions to the user on how to manually launch the agent (on a local or remote target). Or hell, maybe it even has the capability to launch remotely. But the logic you added prohibits that scenario. If using remote peers, the agent launcher logic is avoided altogether...even if the search for matching already-running peers came up empty. John
(In reply to comment #0) > In my case I needed to make an IPeer for a custom IChannel on a local port that > talking to a device-side TCF agent. It doesn't involve launching an agent, and > is not available through the Locator service, and we cannot detect its services > until we actually connect. We had never done this kind of thing before, and it > didn't quite fit into the local vs. remote division. I had to support exactly this scenario. I was able to do it by having the Freescale AbstractFinalLaunchSequence derivative create a Peer object directly with all the attributes I know the peer would have reported had it been discoverable. That seems to me like a cleaner way to support this. That said, I like the addition of selectPeer() in AbstractFinalLaunchSequence. The subclass should be given the opportunity to choose when we find multiple matching peers. It may, e.g., want to prompt the user, or maybe it can decide on its own. However, I don't like how this method allows the derivative to inject some arbitrary peer, as again, there are better ways to support the use case you described above. I propose that we change the design of this method and how it's being used: 1. only call it when there's actually a collection of matched peers to choose from 2. call it regardless of whether the launch wants to consider remote peers or not 3. change the return type to be an int--the index into the array of candidates. The implementation can choose one of the peers by returning its index; if it returns an index outside that range, then it's saying, "use none of these". This could be handy in case the criteria it has set up is not final--it's more of a starting point. I see two reasonable use cases for this: - the decision making is dynamic; needs run-time considerations - the decision making is complex; e.g., does the IP address of the peer fall within a particular subnet? This sort of criteria can't be specific by providing a set of attributes.
(In reply to comment #1) > I had to support exactly this scenario. I was able to do it by having the > Freescale AbstractFinalLaunchSequence derivative create a Peer object directly > with all the attributes I know the peer would have reported had it been > discoverable. That seems to me like a cleaner way to support this. ...oh and the implementation overrode getTCFPeer() to return that peer object.
(In reply to comment #1) > I propose that we change the design of this method and how it's being used: ... > 1. only call it when there's actually a collection of matched peers to choose from > 2. call it regardless of whether the launch wants to consider remote peers or not > 3. change the return type to be an int--the index into the array of candidates. > The implementation can choose one of the peers by returning its index; if it > returns an index outside that range, then it's saying, "use none of these". Why not still return IPeer or null instead of allowing invalid indices as return values? > > However, I don't like how this method allows the derivative to inject some arbitrary peer... >... > ...oh and the implementation overrode getTCFPeer() to return that peer object. ... because it seems like you have to return something created anyway? I'm not sure what you're suggesting as an alternative, by the steps above, or if I'm missing something. If #selectPeer cannot create an IPeer, then the only input I know of comes from TCFServiceManager#getRunningPeers.... But that's not sufficient, because some peers are not running, or cannot be discovered. And we can't add IPeers to that list, except through ITCFAgentLauncher, which as I said, doesn't work for me. How do we come up with the list of matched peers against which to return that int index in your proposal? Again, returning a new IPeer gives an implementor the choice -- if needed -- to inject a new peer, e.g. for non-discoverable ones, or through some UI that challenges the user if it finds that none of the candidate peers supports the right services for debugging. We intend to do this with WLAN at some point, I think.
(In reply to comment #3) > Why not still return IPeer or null instead of allowing invalid indices as > return values? To prevent an implementation from injecting a peer not in the list. Basically, preventing the implementation from pulling a fast one on the caller. After all, the method would be called "choosePeer", not "choosePeerOrGiveMeOneYouWantMeToUse" (which was effectively what is should be called now) :-) > I'm not sure what you're suggesting as an alternative, by the steps above, or > if I'm missing something. If #selectPeer cannot create an IPeer, then the only > input I know of comes from TCFServiceManager#getRunningPeers.... But that's > not sufficient, because some peers are not running, or cannot be discovered. > And we can't add IPeers to that list, except through ITCFAgentLauncher, which > as I said, doesn't work for me. How do we come up with the list of matched > peers against which to return that int index in your proposal? > > Again, returning a new IPeer gives an implementor the choice -- if needed -- to > inject a new peer, e.g. for non-discoverable ones, or through some UI that > challenges the user if it finds that none of the candidate peers supports the > right services for debugging. We intend to do this with WLAN at some point, I > think. By overriding AbstractFinalLaunchSequence.getTCFPeer(). Note that findPeer() merely finds the peer and shoves it into 'tcfPeer'. That field is never accessed directly other than to set it. Anytime the peer is needed, getTCFPeer() is called. Thus, the subclass has the ability to inject an arbitrary peer simply by overriding that method. Now, it should inject a singleton fabricated peer, so that disposal is not a concern. In the Freescale AbstractFinalLaunchSequence subclass (back when our agent wasn't participating in discovery), I fabricated the peer in the constructor. The constructor builds up the steps but doesn't invoke them, so creating the peer there ensures it's available before the launch sequence needs it.
Created attachment 172693 [details] Solution to this bug, 317530 and 317640 Ed, please review.
(In reply to comment #5) > Created an attachment (id=172693) [details] > Solution to this bug, 317530 and 317640 > > Ed, please review. Hmm, this still needs some tweaks. I tried to adapt to this model and found we needed some (overdue) changes to augment our required peer attributes map to distinguish between available peers and avoid launching irrelevant peers (as done in the middle part of #findPeer). Forour agents, I just augmented the required peer attributes with, e.g., IPeer.ATTR_IP_HOST and IPeer.ATTR_TRANSPORT_NAME to cull the undesired agent launchers from the list. But the #specifyRequiredPeer call is done early in the constructor to AbstractFinalLaunchSequence, before 'launch' or 'useLocalAgentOnly' is set, which makes it impossible to set up the required attributes in time. If that call were moved to the end of the constructor, that would be better. Also, the #isInLocalAgent() call does not work for non-TCP/IP connections. I posit that if ATTR_IP_HOST is null or blank, this is the same as a local connection. With these changes, I was able to use local serial, remote non-launched WLAN, and the local launched agents properly. Note, I think it may be unnecessary to have the local/remote/anywhere key added to ITCFAgentLauncher, especially since with your changes, it's just a way to avoid launching "remote" agents. It shouldn't really matter if they're "local" or "remote" -- we currently launch locally in all cases I know of. If there are actually distinctions to be made here, e.g. to avoid the pain of launching something on a remote machine via SSH, I recommend that the requiredPeerAttributes use an appropriate transport name (like "SSH") or be augmented with synthetic (non-TCF) attributes to convey this. It should obviate some of the API changes, I'd think. The patch (to be attached) has all the changes in your original one, so the diffs might be hard to spot. They are: public static boolean isInLocalAgent(IPeer peer) { assert Protocol.isDispatchThread(); String ipAddr = peer.getAttributes().get(IPeer.ATTR_IP_HOST); - return (localIPAddresses.contains(ipAddr)); + return ipAddr == null || ipAttr.length() == 0 || (localIPAddresses.contains(ipAddr)); } --------------- public AbstractFinalLaunchSequence(DsfExecutor executor, EDCLaunch launch, IProgressMonitor pm, - String sequenceName, String abortName) { + String sequenceName, String abortName, boolean useLocalAgentOnly) { super(executor, pm, sequenceName, abortName); - specifyRequiredPeer(); this.launch = launch; + this.useLocalAgentOnly = useLocalAgentOnly; + specifyRequiredPeer(); } ----------
(Actually, I won't upload a new patch.)
(In reply to comment #6) > But the #specifyRequiredPeer call is done early in the constructor to > AbstractFinalLaunchSequence, before 'launch' or 'useLocalAgentOnly' is set, > which makes it impossible to set up the required attributes in time. If that > call were moved to the end of the constructor, that would be better. Sounds good to me. > Also, the #isInLocalAgent() call does not work for non-TCP/IP connections. I > posit that if ATTR_IP_HOST is null or blank, this is the same as a local > connection. That logic was already there (I just changed the name). I guess it depends on what we consider a "local agent". To me, that's an agent running on the same machine as that TCF code (i.e., a Windows or Linux machine). For your serial connection, is the agent running at the end of the serial connection or is it code running on the local machine? > Note, I think it may be unnecessary to have the local/remote/anywhere key added > to ITCFAgentLauncher, especially since with your changes, it's just a way to > avoid launching "remote" agents. > > It shouldn't really matter if they're "local" or "remote" -- we currently > launch locally in all cases I know of. If there are actually distinctions to > be made here, e.g. to avoid the pain of launching something on a remote machine > via SSH, I recommend that the requiredPeerAttributes use an appropriate > transport name (like "SSH") or be augmented with synthetic (non-TCF) attributes > to convey this. It should obviate some of the API changes, I'd think. Fair enough. I'll remove that part of it. We can always introduce that sort of thing if and when we have an actual need for it.
(In reply to comment #8) > > Also, the #isInLocalAgent() call does not work for non-TCP/IP connections. I > > posit that if ATTR_IP_HOST is null or blank, this is the same as a local > > connection. > > That logic was already there (I just changed the name). I guess it depends on > what we consider a "local agent". To me, that's an agent running on the same > machine as that TCF code (i.e., a Windows or Linux machine). For your serial > connection, is the agent running at the end of the serial connection or is it > code running on the local machine? The agent runs on the target, at the end of the connection. But the peer is created locally, because it is not discovered. Do you mean I would have had to say "TCFServiceManager#findSuitableAgentLaunchers(.... localAgentsOnly=false) " in order to find the peer that I created locally? I hope not. As it is, the peer I want will already be in the locator and immediately applicable, but for that #isInLocalAgent check. That's why I added the call (see comment #6) to say that if the peer has no concept of ATTR_IP_HOST, it must be local or created locally.
(In reply to comment #9) > The agent runs on the target, at the end of the connection. But the peer is > created locally, because it is not discovered. > > Do you mean I would have had to say > "TCFServiceManager#findSuitableAgentLaunchers(.... localAgentsOnly=false) " in > order to find the peer that I created locally? I hope not. As it is, the peer > I want will already be in the locator and immediately applicable, but for that > #isInLocalAgent check. That's why I added the call (see comment #6) to say > that if the peer has no concept of ATTR_IP_HOST, it must be local or created > locally. I'm confused. I understand now that you have an agent sitting at the end of the serial connection. I understand that it's not discoverable so you create a peer to simulate what would have occurred if the agent had been discovered. OK, the way I see it, you pass useLocalAgentOnly=false to the AbstractFinalLaunchSequence constructor (from your derived class), since, after all, the launcher is using an agent that's not running on the local machine. Later in the call to AbstractFinalLaunchSequence.findPeer(), wouldn't the peer be found in the first check, since you said the peer you fabricated is already in the locator?
I was just going to reply :) Yes, if I pass useLocalAgentsOnly=false, I can find my manually injected IPeer.
Created attachment 172961 [details] Updated patch based on Ed's feedback I moved the calling of specifyRequiredPeers() and backed out the enhancement to the agent-launcher API (can reintroduce if and when it's needed down the road). Ed, I believe that covers everything?
Looks good, except... these changes require a corresponding change in org.eclipse.cdt.debug.edc.tests.TestUtils#hasTCFAgentLauncher(). Or is that method just here in Austin? And a nit -- I notice you added #log() methods to EDCDebugger but we are using a class called MessageLogger for this purpose (EDCDebugger#getMessageLogger().log(...)). This class has the same basic calling signature but helps make for cleaner reports in the error log, especially by collapsing redundant Exceptions-which-are-CoreExceptions-holding-IStatus-holding-a-null-message-plus-the-real-Exception type messes. :)
(In reply to comment #13) > Looks good, except... these changes require a corresponding change in > org.eclipse.cdt.debug.edc.tests.TestUtils#hasTCFAgentLauncher(). Or is that > method just here in Austin? That class is not in the eclipse.org repository. I'd asked Ken about the junit stuff for EDC. I think he said he was planning on contributing it, but it looks like that hasn't happened yet. > > And a nit -- I notice you added #log() methods to EDCDebugger but we are using > a class called MessageLogger for this purpose > (EDCDebugger#getMessageLogger().log(...)). This class has the same basic > calling signature but helps make for cleaner reports in the error log, > especially by collapsing redundant > Exceptions-which-are-CoreExceptions-holding-IStatus-holding-a-null-message-plus-the-real-Exception > type messes. :) Valid nit. I didn't notice that logger. I've changed the code to use it. Committed to HEAD. Thanks for the review.
*** cdt cvs genie on behalf of jcortell *** Bug 317601: TCF peer discovery, selection, and connection process needs work [*] WindowsFinalLaunchSequence.java 1.14 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.windows/src/org/eclipse/cdt/debug/edc/windows/launch/WindowsFinalLaunchSequence.java?root=Tools_Project&r1=1.13&r2=1.14 [*] LinuxFinalLaunchSequence.java 1.12 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.linux.x86/src/org/eclipse/cdt/debug/edc/linux/x86/launch/LinuxFinalLaunchSequence.java?root=Tools_Project&r1=1.11&r2=1.12 [*] TCFDataModel.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.ui/src/org/eclipse/cdt/debug/edc/internal/ui/views/TCFDataModel.java?root=Tools_Project&r1=1.2&r2=1.3 [*] SnapshotLaunchSequence.java 1.6 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/snapshot/SnapshotLaunchSequence.java?root=Tools_Project&r1=1.5&r2=1.6 [*] EDCDebugger.java 1.3 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/EDCDebugger.java?root=Tools_Project&r1=1.2&r2=1.3 [*] TCFServiceManager.java 1.22 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/TCFServiceManager.java?root=Tools_Project&r1=1.21&r2=1.22 [*] AbstractFinalLaunchSequence.java 1.30 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/launch/AbstractFinalLaunchSequence.java?root=Tools_Project&r1=1.29&r2=1.30
(In reply to comment #14) > > Looks good, except... these changes require a corresponding change in > > org.eclipse.cdt.debug.edc.tests.TestUtils#hasTCFAgentLauncher(). Or is that > > method just here in Austin? > > That class is not in the eclipse.org repository. I'd asked Ken about the junit > stuff for EDC. I think he said he was planning on contributing it, but it looks > like that hasn't happened yet. Hmm... I'm checking it out right now and this class is at rev 1.12. I'd say it's there :) org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.tests
BTW, I found another usage issue, which I think requires reopening this bug. The Javadoc for #getTCFPeer() says a subclass can override it to provide an alternate peer, and in such a case, the #initFindPeerStep is not needed. But actually, this means #initFindPeerStep *must not* be included if you override #getTCFPeer, because tcfPeer is private, and there is no setter. #findPeer reads this field to do the short-circuit. Thus if a subclass does choose to override #getTCFPeer(), this step will read a field that a subclass cannot set, and will still do unwanted work. And, there's a usage disconnect between 'tcfPeer' and #getTCFPeer(), since a client must implement #getTCFPeer(), ignore the field, and also remove the initFindPeerStep in order to work as intended. (A client may also choose to have #getTCFPeer() have side effects, meaning it could potentially return different peers at each call (!).) An easy fix is to make 'tcfPeer' protected, but that seems insufficient. It seems it would be cleanest to always require the initFindPeer step -- just like the assertions in other steps say -- and have #findPeer() call a protected method "IPeer selectExplicitPeer()", which returns null by default, and use this to set tcfPeer if needed. If that yields null, then the current discovery-type behavior can be the fallback. #getTCFPeer should be modified to be made final. Then it's very clear where and how the custom behavior should be integrated.
Created attachment 172999 [details] patch for comment 17
Created attachment 173000 [details] patch for TestUtils class
(In reply to comment #16) > Hmm... I'm checking it out right now and this class is at rev 1.12. I'd say > it's there :) > > org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.tests Ah. I see that plugin was added in February. My workspaces were created before then, and since a cvs update doesn't pick up new plugins, I was missing it. Thanks for fixing it.
(In reply to comment #17) > BTW, I found another usage issue, which I think requires reopening this bug. > > The Javadoc for #getTCFPeer() says a subclass can override it to provide an > alternate peer, and in such a case, the #initFindPeerStep is not needed. > > But actually, this means #initFindPeerStep *must not* be included if you > override #getTCFPeer, because tcfPeer is private, and there is no setter. > #findPeer reads this field to do the short-circuit. Thus if a subclass does > choose to override #getTCFPeer(), this step will read a field that a subclass > cannot set, and will still do unwanted work. Am I overlooking something or isn't this as simple as correcting the violation in findPeer() of not accessing 'tcfPeer' directly except when seeing it. See the javadoc /** * The single TCF peer associated with this session. Do not reference this * field explicitly except to set it. Use {@link #getTCFPeer()} */ private IPeer tcfPeer; So, if we change that check to use getTCFPeer(), then findPeer() will short-circuit and all is good, no?
...of not accessing 'tcfPeer' directly except when SETTING it
(In reply to comment #22) > ...of not accessing 'tcfPeer' directly except when SETTING it No one can do this anyway, since it's private. :) If #getTCFPeer can override the peer, there's still a disconnect within initFindPeerStep. If it sets tcfPeer and #getTCFPeer() doesn't actually return it, then what kind of mayhem ensues? I still see definite use cases where we will want #initFindPeerStep but only sometimes provide an explicit peer. That's what my patch addresses.
(In reply to comment #23) > (In reply to comment #22) > > ...of not accessing 'tcfPeer' directly except when SETTING it > > No one can do this anyway, since it's private. :) Huh? The abstract class can. The point is even *it* should not read the field directly, in order to honor the override the subclass provides via the method. > If #getTCFPeer can override the peer, there's still a disconnect within > initFindPeerStep. If it sets tcfPeer and #getTCFPeer() doesn't actually return > it, then what kind of mayhem ensues? I don't see any potential mayhem. First of all, if getTCFPeer() is overridden to provide a fabricated peer, then initFindPeerStep will be a no-op, thus it never will set tcfPeer. And since tcfPeer is never supposed to be read directly, the setting of tcfPeer should be no problem > I still see definite use cases where we will want #initFindPeerStep but only > sometimes provide an explicit peer. That's what my patch addresses. Why can't you just have the override call into the super's implementation in the cases where you don't want to provide an explicit peer. Wouldn't that provide you the behavior you want? I'm sorry. I don't see your change as wrong; I just don't see what additional control it provides, and it introduces an additional method.
> I just don't see what additional control it provides, and it introduces an additional method. Okay, I guess if you make the change in comment #21, this would work, technically. But it's still a bit messy. I'd still have to hack around in this model. #specifyRequiredAttributes is still called much too early for me. I need to do some UI calls and potentially long-running operations to know whether we need an explicit peer, and if we don't have an explicit peer, to know what the required attributes are (we're in a tricky situation where we need to support different agents using runtime checks). Also, this peer selection process can fail and throw exceptions. So in our launch sequence I just have the #specifyRequiredAttributes method as a no-op right now. I've added another Step at startup to do this peer attribute detection work, before initFindPeerStep, so we can have progress and cancellation, as well as a way to report an exception to the user. This populates peerAttributes. Again, this would be cleaner if we had a #selectExplicitPeer() call, because it could throw an exception. This one-time call would be distinct from the #getTCFPeer() call, which is called multiple times, and cannot and should not throw. Also, I think #specifyRequiredAttributes should be called only from #findPeer(), if #getTCFPeer() returns null. This way it's called directly from the one method that uses it, and its use model is clearer. I see our implementations of this method put static attributes in there, not just ones applicable to dynamic peer selection, thus it's a bit confused. (Such subclasses should just set peerAttributes directly, so they work the same whether or not the peer is chosen explicitly.)
(In reply to comment #25) > > I just don't see what additional control it provides, and it introduces an > additional method. > > Okay, I guess if you make the change in comment #21, this would work, > technically. > > But it's still a bit messy. [snip] OK. That makes sense to me now that I understand your use case better. So, no objection. Let's go with your adjustment.
Ed, can you commit your patches? I'm encountering the TestUtils build error now that I have the plugin. Rather than patch it locally, I'd rather just check out the fix. I can apply the patches if you want.
Ok, I did the TestUtils change, but I'm still tweaking the other launch sequence code. (For instance, #specifyRequiredPeer can stay the way it is on HEAD. There's no special reason to move it, as I thought.)
*** cdt cvs genie on behalf of eswartz *** Bug 317601: update test class for recent TCFServiceManager change [*] TestUtils.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.tests/src/org/eclipse/cdt/debug/edc/tests/TestUtils.java?root=Tools_Project&r1=1.12&r2=1.13
Fixed on HEAD. Make some commenting changes as well (do we need to make new versions of patches for things like that?).
(In reply to comment #30) > Fixed on HEAD. Make some commenting changes as well (do we need to make new > versions of patches for things like that?). I sure hope not :-)
*** cdt cvs genie on behalf of eswartz *** Bug 317601: allow client to override a new method #selectExplicitPeer() rather than overloading #getTCFPeer() to inject a custom peer. And use #getTCFPeer() rather than field reference in #findPeer(), since subclass cannot set tcfPeer. This allows it to retain initFindPeerStep, allowing for injecting or selecting a peer at runtime. [*] AbstractFinalLaunchSequence.java 1.32 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/launch/AbstractFinalLaunchSequence.java?root=Tools_Project&r1=1.31&r2=1.32
Hmm, we ran into some serious problems with one of the changes. I think the "useLocalAgentOnly" flag passed to the AbstractFinalLaunchSequence constructor is forcing us to make a decision much too soon for comfort. In our on-device launch delegate, we use a "connection" concept (an identifier in the launch configuration) to decide where to launch. The connection could map to a local peer or a remote peer, depending on either static or run-time decisions. Also, we don't know until one of our early Steps what that connection is, whether it's even running and configured (the user might be asked to choose one, even), or whether it fits into the "local" or "remote" camp. I had selected "false" as a cop-out, but that fail in a highly predictable way when we thought we were connecting only to a local TCP agent... It seems the simplest way around this is to make the "useLocalAgentOnly" field into a non-final field and add a setter method. It doesn't really seem to be vital that to make it read-only since its (internal) uses are only in one method. Do you use the #getUseLocalAgentOnly method somewhere in your product where its read-only state is important?
Created attachment 173714 [details] patch for new setter
(In reply to comment #33) OK. This goes back to my asking whether we really needed a setter method for that. It just seemed like something that would be known early on. But if that's not the case for you guys after all, then by all means, let's reintroduce it. It's just one of those things that feels a bit awkward. The property affects a one-time event for any given instance, and having a method that can be called at any point in time--including after the event has occurred. I wanted to avoid it if it wasn't really used/needed. I might put an assert in there to try to catch when it's misused.
(In reply to comment #35) > It's just one of those things that feels a bit awkward. The property affects a > one-time event for any given instance, and having a method that can be called > at any point in time--including after the event has occurred. I wanted to avoid > it if it wasn't really used/needed. I might put an assert in there to try to > catch when it's misused. This goes back, again, to the original point of this bug. Peer selection is a mess, and this need for dynamic behavior is why :) Maybe the ideal solution is to not have a constructor argument plus a public getter and setter at all, for this "use local agents only" flag. What if there's just a protected getter (boolean useLocalAgentsOnly()) which the subclass must implement? Then it's called only once and the subclass has full control over what to return.
(In reply to comment #36) > Maybe the ideal solution is to not have a constructor argument plus a public > getter and setter at all, for this "use local agents only" flag. What if > there's just a protected getter (boolean useLocalAgentsOnly()) which the > subclass must implement? Then it's called only once and the subclass has full > control over what to return. +1. I would document when the client (subclass) can expect the method to be called by the base class, so as to make it clear that the client is not on the hook for providing an accurate answer up front. Something along the lines of "...This method is called by the base class when it needs to find a suitable peer. The implementation is not expected to know the answer at construction. Basically, this method buys the subclass time for determining whether remote agents should be ignored or not."
Created attachment 173768 [details] patch on top of 173714 Okay, good. Here's the proposed patch.
(In reply to comment #38) > Okay, good. Here's the proposed patch. Looks good to me.
Fixed in HEAD.
*** cdt cvs genie on behalf of eswartz *** Bug 317601: make "local agent only" flag into a single method call for extensibility. [*] WindowsFinalLaunchSequence.java 1.16 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.windows/src/org/eclipse/cdt/debug/edc/windows/launch/WindowsFinalLaunchSequence.java?root=Tools_Project&r1=1.15&r2=1.16 [*] LinuxFinalLaunchSequence.java 1.13 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc.linux.x86/src/org/eclipse/cdt/debug/edc/linux/x86/launch/LinuxFinalLaunchSequence.java?root=Tools_Project&r1=1.12&r2=1.13 [*] SnapshotLaunchSequence.java 1.7 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/internal/snapshot/SnapshotLaunchSequence.java?root=Tools_Project&r1=1.6&r2=1.7 [*] AbstractFinalLaunchSequence.java 1.35 http://dev.eclipse.org/viewcvs/index.cgi/org.eclipse.cdt/edc/org.eclipse.cdt.debug.edc/src/org/eclipse/cdt/debug/edc/launch/AbstractFinalLaunchSequence.java?root=Tools_Project&r1=1.34&r2=1.35
