fix: isNewSocket logic

[Zzocker]

isNewSocket was holding whether the socket already exists to an endpoint. I think the logic for closing the socket in sendMessage should be :

    const socket = await this.resolveSocket({ socketId: endpoint, endpoint, connectionId })

    socket.send(Buffer.from(JSON.stringify(payload)))

    // If the socket was created for this message and we don't have return routing enabled
    // We can close the socket as it shouldn't return messages anymore
    if (socket && !outboundPackage.responseRequested) {
      socket.close()
    }

As per current logic, the socket is closed only when a message has responseRequested=false and a new socket is created just for this message. Let's suppose a message (responseRequested=false and socket already exists), in this case sender won't call socket.close(). But the receiver will close the socket

https://github.com/hyperledger/aries-framework-javascript/blob/1bda3f0733a472b536059cee8d34e25fb04c9f2d/packages/core/src/agent/MessageReceiver.ts#L138-L156

it's true, when the receiver closes the socket from his side, the sender will also close the socket based on socket.onclose event. But the sender might send a message before receiving the socket.onclose event. I got this bug, while I was testing for an edge agent to setup a mediation with an afj mediator. This was console log (at agent/MessageReceiver.tsL#139 and L#154) from the mediator

received message with return routes session_id:  3018a3bb-c37b-4b22-a1f6-66c494fc3d14 <<< This is connection/1.0/request
closing session for session_id:  a1de283b-0e9c-421e-a0d6-c59ac311d048 <<<< This is didcom/trust_ping
received message with return routes session_id:  a1de283b-0e9c-421e-a0d6-c59ac311d048 <<<< this mediate-request.

@TimoGlastra

[genaris]

While I agree on the fix you did (inverted logic of isNewSocket), I don't fully understand what you meant in your PR comment regarding how the logic should be: do you mean that resolveSocket should return a socket object only if there is a previous socket? Currently, when no socket for a given endpoints is found, it creates it (that's why there is that isNewSocket call just before calling it).

Besides the fix proposed here, I think the recent fix #1337 can also help on the issue you have found with your mediator connection. Have you been testing with agents based on the latest main branch state?

[codecov-commenter]

Codecov Report

Merging #1355 (22e2e9c) into main (78ecf1e) will decrease coverage by 0.01%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##             main    #1355      +/-   ##
==========================================
- Coverage   86.55%   86.54%   -0.01%     
==========================================
  Files         778      778              
  Lines       18443    18432      -11     
  Branches     3143     3143              
==========================================
- Hits        15963    15952      -11     
  Misses       2473     2473              
  Partials        7        7              

Impacted Files	Coverage Δ
packages/core/src/transport/WsOutboundTransport.ts	9.09% <0.00%> (ø)
...ckages/indy-sdk/src/dids/IndySdkSovDidRegistrar.ts	91.54% <ø> (-0.12%)	⬇️
...ges/openid4vc-client/src/OpenId4VcClientService.ts	81.94% <ø> (-0.25%)	⬇️
packages/core/src/agent/MessageSender.ts	86.40% <100.00%> (-0.07%)	⬇️
packages/core/src/crypto/WalletKeyPair.ts	78.26% <100.00%> (-0.47%)	⬇️
...ore/src/modules/connections/DidExchangeProtocol.ts	86.82% <100.00%> (-0.07%)	⬇️
...ckages/core/src/modules/dids/domain/DidDocument.ts	96.66% <100.00%> (-0.04%)	⬇️
...ges/core/src/modules/dids/domain/key-type/index.ts	100.00% <100.00%> (ø)
.../src/modules/dids/domain/key-type/keyDidMapping.ts	90.90% <100.00%> (ø)
...e/src/modules/dids/methods/peer/peerDidNumAlgo2.ts	97.77% <100.00%> (-0.03%)	⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Zzocker]

While I agree on the fix you did (inverted logic of isNewSocket), I don't fully understand what you meant in your PR comment regarding how the logic should be: do you mean that resolveSocket should return a socket object only if there is a previous socket? Currently, when no socket for a given endpoints is found, it creates it (that's why there is that isNewSocket call just before calling it).

Besides the fix proposed here, I think the recent fix #1337 can also help on the issue you have found with your mediator connection. Have you been testing with agents based on the latest main branch state?

The logic I'm suggesting is that we don't check for isNewSocket to be true while closing the socket, we only check
socket != undefined && !outboundPackage.responseRequested.

I tried with the new code and got same logs.

received a message via session_id:  6fd05c3a-9d76-4af0-991b-fe4c24c83657
closing session_id:  5b255cdb-b4b0-4a9e-ad35-c4f2dfcee73f
received a message via session_id:  5b255cdb-b4b0-4a9e-ad35-c4f2dfcee73f

[TimoGlastra]

Comes down to, is return routing for a session until a message is sent without return routing, or is return routing enabled for a session if the first message has return routing

[TimoGlastra]

Hi @Zzocker! Can you update the branch?

[Zzocker]

Hello @TimoGlastra, Done. Will there be any new bug-fix version released after this is merged?