Early-response API feedback

[soxtoby]

A common issue with the current handler API is that SlackNet waits for all of the handlers' work to complete before responding to Slack, which doesn't wait very long before showing errors in the UI. The v0.7 release includes an experimental API for responding to Slack before completing all the work a handler performs. This lets you implement a handler like this:

public Task Handle(SlashCommand command, Responder<SlashCommandResponse> respond)
{
    // Provides feedback as quickly as possible
    await respond(new SlashCommandResponse {
        Message = new Message { Text = "Command received" } });

    // Posts more information once it's available
    var info = await FetchSomeInfoFromARemoteService();
    await _slackApi.Chat.PostMessage(new Message { 
        Text = $"Here's what you asked for: {info}" });
}

Eventually I want to replace the existing API, rather than having two parallel handler APIs, and so the goals are:

• Limit the cost of upgrading
• Make it clear how to respond to Slack
• Handlers that respond early should be easy to write, with minimal cognitive overhead

I'd appreciate any feedback or suggestions people may have.

Other options that were considered

Respond method on the input

public async Task Handle(SlashCommand command)
{
    await command.Respond(new SlashCommandResponse { Message = new Message() });
    await DoSomeMoreWork();
}

Simpler than a Responder callback parameter, but I'm not sure it's obvious enough how to respond.

Append extra work to the return value

public async Task<SlashCommandResponse> Handle(SlashCommand command)
{
    return new SlashCommandResponse { Message = new Message() }
        .AndThen(async () => await DoSomeMoreWork());
}

For handlers with a return value, this keeps the same method signature, which means zero upgrade pain. Doesn't work so well for handlers without a return value, like block action handlers. Having to split the extra work into another function isn't great, either.

Specific return type

public async Task<AsyncResponse<SlashCommandResponse>> Handle(SlashCommand command)
{
    return new AsyncResponse<SlashCommandResponse>
        new SlashCommandResponse { Message = new Message() },
        async () => await DoSomeMoreWork());
}

This makes it really clear how to do more work after responding, but it's pretty ugly, and once again you need to split the extra work into another function.

Async enumerables

public async IAsyncEnumerable<SlashCommandResponse> Handle(SlashCommand command)
{
    yield return new SlashCommandResponse { Message = new Message() };
    await DoSomeMoreWork();
}

I really liked the elegance of this, but it's not clear that you're only meant to return one response, and development on older .NET Core/Framework versions with C# 7 is not straightforward.

Concerns with the current design

• While the general structure of handler code remains the same, upgrading handlers would require removing the return type, adding the new Responder parameter, and calling respond instead of returning, which would be a pain if you have a lot of handlers. Could be mitigated with a code analyzer + fix.
• What happens when you call respond multiple times?
  • Should it throw or just ignore subsequent calls?
  • Requests such as block actions are only waiting for an OK response, and can be handled by multiple handlers - should the first response win, or should SlackNet wait for every handler to respond before sending back the OK response?
• The Responder delegate is simple, but not very extensible. An IResponder interface would make the code slightly more verbose (responder.Respond() instead of just respond()), but it would allow adding new response options in an obvious place. Delegates can have extension methods, which would cover most scenarios, but it might not be obvious enough that they're available.

Once again, any feedback is appreciated. Everything in this API is in an Experimental namespace and marked as Obsolete, because I want to be able to make improvements before "releasing" it.

[mdiluz]

Testing out the new API myself, so might have some feedback down the line, but I think I've spotted a bug currently breaking the non-async command handler. Likely caused by this line: https://github.com/soxtoby/SlackNet/blob/master/SlackNet/Handlers/SlashCommandHandlerAsyncWrapper.cs#L12

I noticed that the SlashCommandResponse I returned from a Handle method wasn't triggering a message in slack. I believe it's because the delegate above is now ignoring the response contained in the non-async methods returned Task. Swapping to the experimental API fixed the issue.

I can report this elsewhere as well if need be.

[soxtoby]

Thanks for the feedback. I've fixed the slash command response being passed along in v0.9.2.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[diegosasw]

It's not very clear how this works now.
Message handlers (e.g: /slack/event) seem to be void (return Task) and seem to respond 200 OK back to Slack immediately without waiting for the message to be handled.

Slack API says that it waits 3 seconds for an answer, which is plenty of time to do something with the event message.

I would rely on Slack retry with exponential wait time mechanism described here

Your app should respond to the event request with an HTTP 2xx within three seconds. If it does not, we'll consider the event delivery attempt failed. After a failure, we'll retry three times, backing off exponentially.

Even if it's a good practice to respond 200 OK asap, it seems there is an important risk currently if an event fails to be placed in a queue or similar for later processing. The 200 OK is returned regardless.

The following handler would execute, but still a 200 OK would be returned.

public class SlackEventHandler
    : IEventHandler<MessageEvent>
{
    private readonly ISlackApiClient _slack;
    
    public SlackEventHandler(ISlackApiClient slack) => _slack = slack;
    public async Task Handle(MessageEvent messageEvent)
    {
        throw new Exception("What if pushing the message into a queue fails here?");
    }
}

The handlers should be awaited before sending 200 OK or error back to slack. It is up to the developer to make sure they are handled quickly and it's recommended they are queued for async processing. But it should wait for the handler to complete.

[ezarzone]

I try to use this, but

        await _slackApiClient.Chat.PostMessage(new Message
        {
            Text = $"Here's what you asked for:"
        });

Doesnt send?

Regards,

[soxtoby]

Hi @ezarzone, this problem is unrelated to the early-response API specifically, so I've responded in the other issue you've raised.

Cheers.