[WIP] Specific optional SessionId option to listen to with SessionHandler

[alberthoekstra]

This PR provides another setting on the SessionHandlerOptions class so you can specify a single SessionId when registering a SessionHandler.

Couldn't find the CONTRIBUTING.md file. Please link so I can check that out.

I've made this change based on this issue.

I'll have to look into the test coverage for the changes.

Extended general description

So basically what I needed was a handler that handles 1 specific session id.

I made an application that does a request for data by sending a message to a specific topic. The response will be sent with the ReplyToSessionId value from the original message into another queue (Response queue).

So the application that needs to receive the message just has to listen to messages in the response queue with its SessionId. Registering a session handler on a Queue client doesn't allow me to filter on a specific SessionId.

Thinking that it would be a waste of resources to just create an if statement in the message handler I found out it was easy to implement it into the project.

Providing functionality with this PR

The best way to show what is possible with this PR is by code. First the current way of solving the problem:

 const string SessionId = "B1A405E2-C354-4F00-8E9E-8D71BA5C30B3";
var queueClient = new QueueClient(ConnectionString, QueueName);

queueClient.RegisterSessionHandler((session, message, arg3) =>
{
    if (!session.SessionId.Equals(SessionId, StringComparison.OrdinalIgnoreCase))
        return Task.CompletedTask;

    // process data here

    return Task.CompletedTask;
}, new SessionHandlerOptions(args =>
{
    // handle error here
    return Task.CompletedTask;
}));

New way of solving the problem:

const string SessionId = "B1A405E2-C354-4F00-8E9E-8D71BA5C30B3";
var queueClient = new QueueClient(ConnectionString, QueueName);

queueClient.RegisterSessionHandler((session, message, arg3) =>
{
    // process data here

    return Task.CompletedTask;
}, new SessionHandlerOptions(args =>
{
    // handle error here
    return Task.CompletedTask;
})
{
    SessionId = SessionId
});

[msftclas]

All CLA requirements met.

[alberthoekstra]

@seanfeldman can you help me out with the ApiApprovals test? The file is not getting included into my project when I’m opening the solution in VS.

It has something to do with the target framework but I can’t figure out what the problem is exactly.

[SeanFeldman]

@alberthoekstra I can do that on your PR, but would need to have permissions to push to your repo.

[SeanFeldman]

Note: once PR #625 is merged into dev, this branch would need to be rebased to include the changes.

[alberthoekstra]

PR #625 is merged.

I've added you as Collaborator in the fork repo.

[SeanFeldman]

Public API changes approved here

[SeanFeldman]

@alberthoekstra all looks good. This will be either released in the next minor or major, depending on the schedule. Thank you.

[alberthoekstra]

@seanfeldman nice! Looking forward for the new version. Thank you for your help.

[SeanFeldman]

The sessionPump is built in a way that if there is no message received within the sessionHandlerOptions.MessageWaitTimeout time, we will close the session and then move on to the next session. With you current change, it will end up closing and reconnecting the session when there are no messages.

Wouldn't that mean CloseSessionIfNeededAsync() would have to take into consideration sessionOptions.SessionId as well?

Agree, this needs to be "polished". I'd also say requires a test to back it up.

[alberthoekstra]

Okay, sorry for the delay. But I've got it working.

In the MessageSession class, the OnMessageHandler method was overridden from the base class. It just threw an exception saying it's not supported.

I've removed that override and it now simply works like expected.

I'm sure I'm missing something because this is way too simple. I couldn't find the reason why it threw the exception in the override, maybe one of you can remember?

[nemakam]

This seems to be a simpler approach. There are few more things to think about though. The ReceivePump starts an async background task for every message to renew the message lock in case the pump is expected to renew messages. In case of session, that would throw since message locks need not be renewed. Instead, session lock needs to be renewed. So there needs to be only one background task which renews the session. But what is the termination condition? How long should it renew? Should it renew continuously till we call CloseAsync()?

[nemakam]

After fixing the above mentioend issue, @alberthoekstra , could you do a manual test once in your machine and let this Handler run for say 15 minutes when there are messages to be consumed, and another 15 minutes when there are no messages to be consumed. Check for any errors that pop up during that time.

[alberthoekstra]

This is getting too complicated for me. I can't get a hang of the structure in the project so I think I'll have to call it. Maybe someone else can manage these issues.

[SeanFeldman]

@alberthoekstra it looks like you're very close to complete this PR. Are you sure you don't want to run that manual test to ensure all works?

[alberthoekstra]

The manual tests are not the only things @nemakam suggested right? I think the change i've made should be reverted and there should be an own receive pump somewhere.

[SeanFeldman]

Marked PR as [WIP] until it's not and ready to be merged.

[axisc]

@alberthoekstra we're closing this PR given that this repo has been migrated to Azure SDK for .NET.

If you feel that you still want to pursue this feature, please open an issue in the new repo and we can discuss this further.