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

broadcastEvent on no channel #1310

Closed
robmoffat opened this issue Aug 5, 2024 · 7 comments
Closed

broadcastEvent on no channel #1310

robmoffat opened this issue Aug 5, 2024 · 7 comments
Assignees
Labels
bug Something isn't working FDC3 for Web Browsers

Comments

@robmoffat
Copy link
Member

Minor Issue

When an app (A) is opened with context and adds it's context listener it probably won't have a user channel set.

The desktop agent is required to send a broadcastEvent to the app with the context, but currently requires a channelId to be set in the schema.

Is this right or not?

"BroadcastEventPayload": {
			"title": "broadcast Event Payload",
			"type": "object",
			"properties": {
				"channelId": {
					"title": "channel Id",
					"description": "The Id of the channel that the broadcast was sent on.",
					"type": "string"
				},
				"context": {
					"$ref": "../context/context.schema.json",
					"title": "Context",
					"description": "The context object that was broadcast."
				},
				"originatingApp": {
					"title": "Originating AppIdentifier",
					"description": "Details of the application instance that broadcast the context",
					"$ref": "api.schema.json#/definitions/AppIdentifier"
				}
			},
			"additionalProperties": false,
			"required": [
				"channelId", "context"
			]
@robmoffat robmoffat added the bug Something isn't working label Aug 5, 2024
@Roaders
Copy link
Contributor

Roaders commented Aug 20, 2024

Can I have some more details on how this is supposed to work?

I assume that the broadcast message is only received by the newly opened app if it calls addContextListener on boot?

Also how is it supposed to work with the channels as Rob is asking above.

Thanks

@bingenito
Copy link
Member

I never thought of this workflow myself for open(). I always assumed the workflow is open(no context), app starts, performs the addIntentListener and gets the pending context based on the intent.

@kriswest
Copy link
Contributor

@Roaders @robmoffat there was some discussion of this over email while I was on vacation - I forget who was on that email.

@robmoffat channelId needs to be nullable in the schema - I'll fix that. A broadcast received due to a context passed to fdc3.open would not have a channel - I think an explicit null should represent that (rather than some arbitrary strong constant.

@Roaders Essentially, if an app is opened via fdc3.open with a context passed, it is then passed to a context listener added by the app, after it has been added. This means that the Desktop Agent has to sit and wait for an appropriate context listener to be added (and you have to consider that they might add more than one - it has to be passed to each they add that would/should receive it).

This is the same as what happens with intents, theres just no intent passed along and we use a ContextHandler (added via fdc3.addContextListener) rather than an IntentHandler (added via fdc3.addIntentListener).

I will happily say I don't think much of this part of the FDC3 API design as there is some unnecessary complexity in the waiting around for the listener to be added and I dislike the reuse of the listener for a slightly different purpose (it has no way to tell if it came off a channel or via open) but its been there a long time.

In the protocol, we'll need to document that the app should be opened, the Desktop Agent should wait appropriate Listener(s) to be added and then send a specific broadcastEvent message only to that app to deliver the context. No special code in the proxy should be needed as broadcastEvent's received should already get routed to the appropriate listeners.

Please note that there are some timeouts for adding the listeners specified - this was my attempt to put some bounds on the waiting, see:

In the end what we added was:

An FDC3 Standard compliant Desktop Agent implementation MUST:

https://fdc3.finos.org/docs/api/spec#desktop-agent-api-standard-compliance

An FDC3 Standard compliant application that supports the use of context data SHOULD:

  • Where an app is intended to receive context from fdc3.open calls, use the fdc3.addContextListener API call to set up appropriate handlers within 15 seconds of the application launch (the minimum timeout Desktop Agents are required to provide) in order to be widely compatible with Desktop Agent implementations.

https://fdc3.finos.org/docs/context/spec#context-data-standard-compliance

An FDC3 Standard compliant application that supports intents SHOULD:

  • Where an app is intended to be launched in order to resolve a raised intent, use the fdc3.addIntentListener API call to set up the necessary handler function(s) within 15 seconds of the application launch (the minimum timeout Desktop Agents are required to provide) in order to be widely compatible with Desktop Agent implementations.

https://fdc3.finos.org/docs/intents/spec#intents-standard-compliance

Finally, please note this error that can be retruned from fdc3.open:

enum OpenError {
  ...
  
  /** Returned if the specified application launches but fails to add a context
   *  listener in order to receive the context passed to the `fdc3.open` call.
   */
  AppTimeout = 'AppTimeout',
}

Its existence implies that you don't resolve fdc3.open's Promise until the context has been delivered (at least once - the whole multiple listeners thing isn't well thought through here) or you timeout and reject.

@Roaders
Copy link
Contributor

Roaders commented Aug 20, 2024

Thanks for the explanation.

No special code in the proxy should be needed as broadcastEvent's received should already get routed to the appropriate listeners.

No special code other than having to act on a message with no channelId. As I have mentioned in other threads we've been writing our agent with the assumption that message routing might be dumb - i.e. all messages going to all agents and the agent then deciding if it's interested in the message. It's becoming increasingly clear that the protocol has not been writen in a way that supports this.

Basically I think the way we need to write it is to do no filtering of messages on the agents and no checking that the raiseIntent message for example is intended for that particular agent. If we get a message we act on it I guess.

Our main implementation is for the correct "smart" message routing so that all messages are only sent to the intended agent. We are allowing the messaging to be swappable though so some users might implement a stupid system that does not route messages. By the sounds of it we need to remove that requirement.

@kriswest
Copy link
Contributor

kriswest commented Aug 20, 2024

As I have mentioned in other threads we've been writing our agent with the assumption that message routing might be dumb - i.e. all messages going to all agents and the agent then deciding if it's interested in the message. It's becoming increasingly clear that the protocol has not been writen in a way that supports this.

Correct - and to be clear that applies to the API standard, not just this protocol supporting it. It's rarely a good idea to send everything everywhere - its harder to secure, it creates additional traffic and processing to do in threads/apps that wouldn't need to do any otherwise, affecting performance and (your ability to implement) security.

Returning to terminology for a second, there is conceptually a single 'agent' with an 'API endpoint' for talking to it in each application, rather than an 'agent' in each. There is this line in the API overview about a Desktop Agent:

A Desktop Agent is a desktop component (or aggregate of components) that serves as a launcher and message router (broker) for applications in its domain.

Note applications is plural and routes messages between them (rather than just filtering whats on a bus). I hope that helps provide some background.

@kriswest
Copy link
Contributor

@robmoffat and @Roaders see a42798c - this should fix the issue with channelId on broadcastEvent. channelId MUST be set, but set it null for an fdc3.open broadcast.

THis will need documenting in the communication protocol, hoping to get onto that this week, once I've got through your backlog of questions/comments.

@kriswest
Copy link
Contributor

Closing on the assumption that the fix is good, reopen if not!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working FDC3 for Web Browsers
Projects
None yet
Development

No branches or pull requests

4 participants