Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarification re repeated addContextListener and addIntentListener calls #1394

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

kemerava
Copy link
Contributor

Closes #1390

Based on the discussion on the issue, adding the clarification to the standard on the expected behavior when multiple calls are made to addContextListener and addIntentListener

@michael-bowen-sc

@kemerava kemerava requested a review from a team as a code owner October 18, 2024 16:18
Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for fdc3 canceled.

Name Link
🔨 Latest commit 278d71b
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/67445c05aa21e60008d7ae80

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Personally, I think that we triggering of all listeners is currently a MUST and should remain so (meaning the SHOULDs in the this PR become MUSTs to explicitly state that).

I also wonder if we need to add a comment about reconnection. If a web app is refreshed it needs to add new handlers as the old ones will be fone - the DA should be tracking the lifecycle of the app and considering them to have been removed if the app reloads/navigates/reconnects. That generally only applies to web apps as most others don't 'reconnect' or 'refresh' in a way that would wipe out their handler functions

docs/api/ref/Channel.md Outdated Show resolved Hide resolved
docs/api/ref/DesktopAgent.md Outdated Show resolved Hide resolved
docs/api/spec.md Outdated Show resolved Hide resolved
src/api/Channel.ts Outdated Show resolved Hide resolved
src/api/DesktopAgent.ts Outdated Show resolved Hide resolved
src/api/Errors.ts Outdated Show resolved Hide resolved
@bingenito
Copy link
Member

I see a mix of resolvererror and resolveerror in text and anchor links.

src/api/DesktopAgent.ts Outdated Show resolved Hide resolved
@kriswest
Copy link
Contributor

I see a mix of resolvererror and resolveerror in text and anchor links.

Good 👀 @bingenito - I missed several of those. Added to suggestions

@kemerava
Copy link
Contributor Author

Good job! Personally, I think that we triggering of all listeners is currently a MUST and should remain so (meaning the SHOULDs in the this PR become MUSTs to explicitly state that).

I also wonder if we need to add a comment about reconnection. If a web app is refreshed it needs to add new handlers as the old ones will be fone - the DA should be tracking the lifecycle of the app and considering them to have been removed if the app reloads/navigates/reconnects. That generally only applies to web apps as most others don't 'reconnect' or 'refresh' in a way that would wipe out their handler functions

@kriswest In terms of the reconnection, I think that there a case which can to be made for Desktop Agent to keep the listeners in memory at least for a short period of time. Not sure as much for web apps, but for others, depending on which method of connection is used, it is possible that the app temporarily lost the connection, and I am not sure if it makes sense to always ask apps to reconnect.
This is my scenario, for example, if Desktop Agent is determining the app connection based on the heartbeats from the app, and then the app disconnects, then the behavior is slightly undererministic, where Desktop Agent might have already "noticed" that the app has disconnected, and removed the listeners, or it has not, and then the listeners are still there, and then the app unnecessarily sending more calls to DA. Of course, even with keeping listeners in memory we cannot guarantee for sure if that senario would not happen. We can also think about introducing an endpoint which would list all the listenerIds which DA has for the given app, this way the app can verify if it is missing some listeners or if there are some which are no longer needed and can be cleaned up
Please let me know what you think!

bingenito
bingenito previously approved these changes Oct 25, 2024
@kemerava kemerava requested a review from kriswest November 22, 2024 20:06
@kriswest
Copy link
Contributor

@kriswest In terms of the reconnection, I think that there a case which can to be made for Desktop Agent to keep the listeners in memory at least for a short period of time. Not sure as much for web apps, but for others, depending on which method of connection is used, it is possible that the app temporarily lost the connection, and I am not sure if it makes sense to always ask apps to reconnect.
This is my scenario, for example, if Desktop Agent is determining the app connection based on the heartbeats from the app, and then the app disconnects, then the behavior is slightly undererministic, where Desktop Agent might have already "noticed" that the app has disconnected, and removed the listeners, or it has not, and then the listeners are still there, and then the app unnecessarily sending more calls to DA. Of course, even with keeping listeners in memory we cannot guarantee for sure if that senario would not happen. We can also think about introducing an endpoint which would list all the listenerIds which DA has for the given app, this way the app can verify if it is missing some listeners or if there are some which are no longer needed and can be cleaned up
Please let me know what you think!

Apologies, for the delay in responding to your question on this. Disconnection is an awkward topic for FDC3 as what it means could vary greatly between implementations. In the classic container implementations for web apps, its doesn't really happen other than via the lifecycle of the application. That's because the Desktop Agent tends to be provided by a local service of some sort and is therefore not affected by the network connection... If it goes down, the Desktop Agent API is still available (the connection between the app and Desktop Agent is unaffected), listeners remain valid etc..

I suspect that this will be the same for most FDC3 for the Web implementations... if the Desktop Agent was already loaded and connected to, then the network connection going down doesn't affect its ability to provide interoperability between windows! Of course, there will be exceptions: Desktop Agent implementations which rely on a remote web service and situations where the Desktop Agent window is refreshed and reloads. I suspect these cases need thinking about. In both cases there is potentially a need for some sort of event to pass back for either the apps or the getAgent code to handle. A new connection to the DA needs to be set up (as we'll lose the Message Ports in the later case).

I think we'd be well advised to trigger the connection flow afresh - the DA might come back with a different config for apps for example. The alternative is that the Desktop Agent. Whether the getAgent code retains listener or requires new ones to be set-up at that point is something we'll need to consider. getAgent() will certainly need to re-register the listeners with the DA, even if the app doesn't have to re-add them.

When it comes to native apps and disconnection, we will usually be talking about a local service that provides access to the DA going down or being restarted. In those cases, new connections (and polling for connection) will likely be needed. Whether listeners need to be readded or not feels like something we should discuss as it should probably be implemented consistently (and hence handled in a standardized way).

At the very least we'll need to think about adding an (addEventListener) event for disconnect and reconnect and then discuss how that should be handled. If what I've said here makes sense, would you be up for raising an issue for the Standards Working Group to discuss? It might help to enumerate all the different types of disconnection that could occur and figure out which need solutions (I think the DA going away, at least temporarily, is the primary one to address).

@kriswest
Copy link
Contributor

P.S. I think this PR is ready to go - the only thing holding it up is the fact that we closed the 2.2 scope and this does introduce a new error that MUST be thrown. However, it's not a big change and I would be happy to consider a motion to put it into 2.2 (which is currently blocked by completing the getAgent implementation work) if participants wanted to get it implemented and there were no objections or existing examples of alternative ways of handling intent listener conflicts that another participant wanted to to preserve.

@kemerava
Copy link
Contributor Author

kemerava commented Dec 6, 2024

P.S. I think this PR is ready to go - the only thing holding it up is the fact that we closed the 2.2 scope and this does introduce a new error that MUST be thrown. However, it's not a big change and I would be happy to consider a motion to put it into 2.2 (which is currently blocked by completing the getAgent implementation work) if participants wanted to get it implemented and there were no objections or existing examples of alternative ways of handling intent listener conflicts that another participant wanted to to preserve.

@kriswest has there been consensus on this one? In my view, it is a pretty small change, but if we want to hold it off until 2.3 that is fine as well

@kriswest
Copy link
Contributor

We were set to hold off for 2.3 for purely procedural reasons (we voted to close the scope). However, I agree this is small and fixes an existing problem and could go in for 2.2. This PR could be called a breaking change as it introduces a new error that could be returned on existing calls to raiseIntent and raiseIntentForContext. However, you would previously have been faced with unstandardized behaviour if you tried to add a listener for the same intent multiple times and DAs might have returned a non-standard error, overwritten the handler or something else. Hence, I think bringing this in is a better situation.

Regardless it will need approval by the SWG which we can do at the next meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify: What should happen on repeated listener addition for the same contextType/intent
3 participants