feat: remove MessagePickupRepository injection symbol #2168
Conversation
Signed-off-by: Ariel Gentile <gentilester@gmail.com>
|
There was a problem hiding this comment.
Nice! only one bigger concern re the events and serverless architectures.
Otherwise, would be really interested to also think more about the multi-instance session management. We're now discussing it in mediator repo (not sure what came out of the WG call yesterday?), but it's probably relevant to solve in Credo core (at least provide the needed interfaces).
|
|
||
| // This is an internal event that will be catched by Message Pickup module and queued, in case the | ||
| // Agent supports it | ||
| this.emitQueuePackedMessageForPickupEvent(agentContext, { |
There was a problem hiding this comment.
I generally like this new approach, but one downside is that if no message pickup module is registered the queued messages will be lost. I think if we receive a message that can't be delivered we should probably handle that in some way(maybe we can check if there's any subscribers?)
There was a problem hiding this comment.
Also, one more consideration is that we've noticed that in serverless architectures asynchronous events don't really work well. You don't know when an incoming request is done processing.
That's why I wanted to look into reducing the number of events emitted, but I wasn't sure yet what a good approach would be. I think callbacks make sense, but is also what we had previously (we could of course have no handler by default, and the message pickup module can register it, but then we're basically back at the injection in a different way).
Thoughts?
There was a problem hiding this comment.
Say you don't like this approach without saying you don't like this approach! 😆
Seriously speaking, I think we can solve this problem in different ways, and surely we can avoid using async events for that. If we leave this "Queue Transport repository" as a core feature of DIDComm, we can queue messages synchronously. Also, if we consider mediation (the concept itself, not the coordination protocols) as a core feature, we can also setup there different queue transport policies, like the one you mentioned some time ago about only queuing messages for agents we are a mediator of.
There was a problem hiding this comment.
Say you don't like this approach without saying you don't like this approach! 😆
I really like getting rid of the injection symbols :)
But i've also had some bad experience trying to make credo work in serverless environments and it sadly just didn't work :(
| public constructor(options: MessagePickupModuleConfigOptions<MessagePickupProtocols>) { | ||
| this.options = options | ||
| // Message Pickup queue: use provided one or in-memory one if no injection symbol is yet defined | ||
| this.#messagePickupRepository = options.messagePickupRepository ?? new InMemoryMessagePickupRepository() |
There was a problem hiding this comment.
Should we change this to something like messageQueueRepository? or queuedMessageRepository?
There was a problem hiding this comment.
This subtle question made me think that the approach we are following is not correct: this message queue is part of DIDComm core, regardless of the message pickup mechanism we use (for instance, the infamous "implicit" mode present in some mediators do not use Message Pickup protocol at all, but they do access this element to send queued messages to their clients at the moment they connect to a WebSocket).
In such case, this QueueTransportMessageRepository must be defined in the base DIDComm module configuration, and therefore much of the things done in this PR would not make sense.
There was a problem hiding this comment.
I agree. Routing/forwarding/queuing is part of core. Mediator management and pickup not! :)
| private messages: InMemoryQueuedMessage[] | ||
|
|
||
| public constructor(@inject(InjectionSymbols.Logger) logger: Logger) { | ||
| public constructor(@inject(InjectionSymbols.Logger) logger?: Logger) { |
There was a problem hiding this comment.
If we don't have access to this, we should just pass the agent context to all the methods in this class I think
There was a problem hiding this comment.
Yeah, I didn't want to change all MessagePickupRepository interface methods only for this basic implementation. But I think resistance is futile: a good refactoring of all of this will be needed before 0.6.0 release!
|
Can you add changeset(s) for the changes? |
|
For the moment I'll close this PR in favour of another one where we can refactor DIDComm modules in such a way that core features and concepts are part of the base module, and we leave protocol-specific parts to the corresponding modules. |
This PR removes MessagePickupRepository injection symbol, which makes DIDComm base module to not depend on Message Pickup. Instead,
MessageSenderemits an internal event when a message is meant to be queued for pickup (e.g. when the recipient does have a transport queue endpoint). Such event is catched byMessagePickupModuleand effectively added to the queue there.The main advantage for now (apart from removing an InjectionSymbol) is that we can define a very basic DIDComm Agent that only includes the very basic DIDComm modules (didcomm, oob and connections).
I want to take this as an initial step to refactor the relationship between modules within DIDComm. For instance, I think it would be nice to put in MessagePickupModule the logic for pickup clients that currently resides on
MediationRecipientApi, and maybe we can even reduce the amount of modules by having a single mediation module that will be used for the Mediation Coordination protocol, leaving the base mediation features (records, Forward message handling) to the base DIDComm module, along with connections/oob.