Log Promise rejections in command client

[2colours]

This very banal change allows discord command clients to report Promise rejections with log level "warning" rather than just failing silently. I think it's much better than nothing because unhandled Promises couldn't fall back anywhere so these logging has to be compared to "oops, too bad" silent failure.

Probably something similar should be done (or at least preferred) for other DiscordPHP utilities that feed into the event loop; most notably the Discord class itself. But again, I think it's better than nothing.

[SQKo]

See https://github.com/discord-php/DiscordPHP/pull/1157#issuecomment-2002136886

[2colours]

Regarding to your suggestion, we already considered about it, but it is wrong to log everything even those promise rejections already handled/caught by users and not respecting users configured Logger.

1. it wouldn't log "already handled/caught" promise rejections because it wraps the user function. If the user function handles it, the rejection simply doesn't happen.
2. it absolutely respects the "configured Logger", it uses the logger obtained in the Discord class.

Moreover, I would argue the gazillion of already existing, not really useful or informative debug logs are much harder to justify than any sort of logging of an otherwise uncatchable promise rejection.

Due to the nature of promise chain, where this library being in middle position, the ideal solution has to be implemented on user's end.

Confer point 1 above. The chaining is in fact perfect for the library to handle this, it's not "in the middle" but actually the outer layer.

So we went to a better idea, It is somewhat already implemented but if i recall, it requires your code to be wrapped inside a coroutine yields so your own code throws will be also forwarded

I for one use ReactPHP's async/await. I don't see the advantages of moving my code over to coroutine instead. I don't think safer, debuggable promise rejections should be a rather bulky opt-in thing. My solution works with anything that produces a PromiseInterface.

All in all, it still seems to me that my solution is better than the current situation, all things considered.

[2colours]

It's not true that nobody can even catch the rejection, thats why I previously remind the proper use of then() and done(). Same for this library perspective, if the dependency does not handle promise rejection properly, it can never be caught by us.

I somehow missed this but this is just not true. Of course if somebody sinks the value of a Promise in a usual synchronous function, that cannot be recovered. In that case, my PR still does no damage, mind you; it just cannot remedy because that's a genuine user error. I'm not talking about that. I pass async functions to ReactPHP and await all Promises. It is absolutely ReactPHP's fault that it gets sunk, and that's what this PR is meant to address.

[2colours]

Anyway, the latter part made me realize there was probably some miscommunication.

set_rejection_handler would hook into all Promises ever but that's not what I hope to achieve with DiscordPHP. DiscordPHP should, however, take care of the Promises it creates from user code. That's what this PR addresses for command client handling, and that is indeed DiscordPHP's business.

[SQKo]

My explanation was to handling the rejection in general, not command clients

We are not outer layer, your case with command client just happens to you (user) apparently being the last since you use command callbacks and the promise ends with you.

While I have no time yet to test the code, does this break the flow for people extending the class or somewhat the callback? Which is why I said we are not outer layer, even you as the user might be not the outermost layer.

Perhaps we will cover for only this case and use your code but we add options to disable it?

[2colours]

Sure thing: if the user sinks a Promise, that's the user's fault. However, here the "user" was DiscordPHP itself, sinking the potential Promise coming from the handling.

Also, certainly there are many things that could be tuned about the behavior. I would argue that tuning this is about as justified as tuning what logs get emitted in the first place, though. For my purposes, it's okay, just doesn't sound consistent or practical.

Other than that, I'm inclined to say the code is so simple that one can confidently estimate the effect. After some layers of indirection, $command->handle invokes the function the user provided (coming from DiscordCommandClient::registerCommand -> DiscordCommandClient::buildCommand -> Command::__construct). If this was an "async" function (whatever that produces a PromiseInterface), we can catch that and do something with the rejected value, rather than sink it. If the user didn't register an "async" function, nothing happens. If they did register an "async" function, the last thing we do with it is obtain the error state basically. After that, it's discarded like before. I could be wrong but this seems simple and safe enough honestly.

[SQKo]

Sure thing: if the user sinks a Promise, that's the user's fault. However, here the "user" was DiscordPHP itself, sinking the potential Promise coming from the handling.

As I've stated already, the issue happens on gateway connected, we proven this by using the logic with our REST-only Discord HTTP library to send message reply which just works fine in handling the rejected promises. We are not sure the main root cause but it's most likely from our web socket dependencies. Previously we spotted a memory leak issue from our side, but after it's fixed, the issue remains. But if you mean only the particular message reply inside command handler, yes it is our mistake not properly using done(), a 8 years old code we were not aware.

I'm not against with idea logging promise rejection, as I've done that with the event side (but requires coroutine wrapped logic) that I mentioned before. But I don't want to mishandle the promise chain further. All it needed is to hook a code in middle but still have user able to handle on their end. The message based command client was out of my attention, the command handler wraps in a callback in middle and we can see if user returned back the promise. In which suits the same fix.

I will test your PR and add option to disable it in the command client, I think not many people actually extending the message based command handler so its all ended on their side. They'd still have alternative to just extend from the main discord class if they want.

Also, certainly there are many things that could be tuned about the behavior. I would argue that tuning this is about as justified as tuning what logs get emitted in the first place, though. For my purposes, it's okay, just doesn't sound consistent or practical.

Other than that, I'm inclined to say the code is so simple that one can confidently estimate the effect. After some layers of indirection, $command->handle invokes the function the user provided (coming from DiscordCommandClient::registerCommand -> DiscordCommandClient::buildCommand -> Command::__construct). If this was an "async" function (whatever that produces a PromiseInterface), we can catch that and do something with the rejected value, rather than sink it. If the user didn't register an "async" function, nothing happens. If they did register an "async" function, the last thing we do with it is obtain the error state basically. After that, it's discarded like before. I could be wrong but this seems simple and safe enough honestly.

The flow from buildCommand, __construct, registerCommand is synchronous, only the command handler (callback) is up to the user, the user is not required to return anything in the command callback, even promises, we just provided an optional message reply helper which uses (improper) promises. This is probably what missing in your purposes, plus need to be noted that this version still supports PHP 8.0 without the async function.

[2colours]

I frankly don't know what you are talking about. I think you are over-complicating this a lot.

I'm using the "command client" and noticed that whenever an exception is raised, the bot goes on like nothing happened. There is nothing obscure about this. When you do $command->handle($message, $args), you invoke a potentially async function and you potentially get a Promise stored into $result. When you proceed to do nothing with $result, that's when you silently swallow the error. Nothing about web socket dependencies and memory leaks.

I don't know what the "coroutine wrapping" yields but I think regardless how the user created a function that can return a PromiseInterface compatible value, that should be handled correctly. That could really be a raw chained Promise or in my particular case an "async wrapped" function, and who knows whatever else.

Also, it really should be simple enough that the "async check" is a mere $result instanceof PromiseInterface condition. This won't ever trigger if the user passed a callback that doesn't return, or returns some arbitrary value. Honestly, I think we are spending way too many circles on this.

[SQKo]

I'm using the "command client" and noticed that whenever an exception is raised, the bot goes on like nothing happened. There is nothing obscure about this. When you do $command->handle($message, $args), you invoke a potentially async function and you potentially get a Promise stored into $result. When you proceed to do nothing with $result, that's when you silently swallow the error.

The command handle does not require promise in callback, I already put that in bold if you have code return that in the callback, the issue will happen.

Nothing about web socket dependencies and memory leaks.

This is a response to your issue in the promise v3 PR, completely different.

I don't know what the "coroutine wrapping" yields but I think regardless how the user created a function that can return a PromiseInterface compatible value, that should be handled correctly. That could really be a raw chained Promise or in my particular case an "async wrapped" function, and who knows whatever else. Also, it really should be simple enough that the "async check" is a mere $result instanceof PromiseInterface condition. This won't ever trigger if the user passed a callback that doesn't return, or returns some arbitrary value. Honestly, I think we are spending way too many circles on this.

I'm talking about the reactphp/async library the php 8.1+ with fiber does not have done() at all, it is different behaviour to pre php 8.1 which still uses coroutine without fiber. We do not plan to only support fiber only at this version but it will be in future major version.

I frankly don't know what you are talking about. I think you are over-complicating this a lot.

The whole cause of this is just because the old codes in CommandClient unmaintained like $message->reply() which should be $message->reply()->done(), and $message->channel->sendEmbed() becomes $message->channel->sendEmbed()->done(), not the whole library.

[2colours]

The command handle does not require promise in callback, I already put that in bold if you have code return that in the callback, the issue will happen.

And this is what the PR addresses. If this is well understood by both of us, why do we always circle back to it? For what it's worth, this is not a corner case, this is like 50% of the use cases, at least.

I'm talking about the reactphp/async library the php 8.1+ with fiber does not have done() at all, it is different behaviour to pre php 8.1 which still uses coroutine without fiber. We do not plan to only support fiber only at this version but it will be in future major version.

Okay, for the same major version I think this is fair enough. This PR doesn't break compatibility with any environment that the library worked with so far. It doesn't introduce new dependencies or silent assumptions, just uses the same (outdated) ReactPHP PromiseInterface that the code already depended on, somewhere explicitly, at other places implicitly. Anybody is free to provide a compatible function from user code (just like they did previously, mind you) - Composer will sabotage installing a different version of the dependencies anyway, so users won't even have a different option...

The whole cause of this is just because the old codes in CommandClient unmaintained like $message->reply() which should be $message->reply()->done(), and $message->channel->sendEmbed() becomes $message->channel->sendEmbed()->done(), not the whole library.

I don't know what this "done" does so this doesn't help. All I see is that the issue isn't particularly related to $message->reply(). All Promises DiscordPHP creates should be handled by DiscordPHP, or passed back to the user. Here the latter is not an option. Wrapping $message->reply() as well is only for the sake of consistency. The important bit is "if $result is a Promise, trace the error".

[SQKo]

And this is what the PR addresses. If this is well understood by both of us, why do we always circle back to it? For what it's worth, this is not a corner case, this is like 50% of the use cases, at least.

Yes thats why I said i will test and add your code later, I dont circle back, just repeating what Ive already said, but you seem dont understand and keep generalizing it is DiscordPHP fault in all parts.

Not many people use the command client it is very old and we got unnoticed about improper promise in our side, since almost no one reported (thanks to you).

I don't know what this "done" does so this doesn't help. All I see is that the issue isn't particularly related to $message->reply(). All Promises DiscordPHP creates should be handled by DiscordPHP, or passed back to the user. Here the latter is not an option. Wrapping $message->reply() as well is only for the sake of consistency. The important bit is "if $result is a Promise, trace the error".

This is merely a bug, everyone else in discord already stated in #faq that use of promise need to have done() because reactphp promise v2 will try catch any throwable if it only has then() in the end. Using done() reactphp promise will not catch the throwable and will crash the php process with the traces like usual, and we dont need to care about logs anymore.

[2colours]

I dont circle back, just repeating what Ive already said, but you seem dont understand and keep generalizing it is DiscordPHP fault in all parts.

I don't know how "the value doesn't always have to be a Promise" would counter that but anyway, I don't know what "in all parts" would mean to begin with. I only said that in this particular case, DiscordPHP didn't do what all users of a Promise should do - make sure all paths are handled before discarding the value. And I don't think we argue about that.

This is merely a bug, everyone else in discord already stated in #faq that use of promise need to have done() because reactphp promise v2 will try catch any throwable if it only has then() in the end. Using done() reactphp promise will not catch the throwable and will crash the php process with the traces like usual, and we dont need to care about logs anymore.

I still don't think we should talk about done, it's not needed to understand and solve the issue. You mentioned "crashing the php process with the traces like usual" - now that definitely mustn't happen, though. Sure, there can be bugs in my Discord bot - like all software in general - but it's not acceptable behavior for a Discord bot to crash on an exception, especially when that exception can be a result of a missing permission or a problem not under the control of the bot creator. Even with some proper process management, it's a terrible overhead to repopulate the memory with the required state and reinitiate the Discord connection...

[2colours]

After the disruptive experience on the discord server that I specifically joined to give a chance to a more fruitful discussion, ending with my ban after a rather passive-aggressive tone from the hosts, I'm curious whether you will prove me wrong and have a constructive attitude to a fix.

[SQKo]

Other server moderator banned you from the server from ignoring the warning, you exaggerated an issue that only applies to a old module as a whole library problem and insisted on your own opinion while denying all of everyone else's explanations .

Fixed in https://github.com/discord-php/DiscordPHP/pull/1210 which gives everyone but not limited to you freedom to extends the solution. Thanks for reporting.