Promises are not properly handled (unresolved)

[joelluijmes]

Hey there!

I noticed in the code base a bit of an anti-pattern: unresolved or unhandled promises. Unhandled promises, also known as "fire-and-forget" programming patterns, can be problematic for a few reasons.

• Silent failures: When a promise is not handled (i.e., not await-ed or catch-ed), any error it throws won't be caught. This means that your code could be failing silently without you knowing about it.
• Unhandled promises can trigger UnhandledPromiseRejectionWarning in nodejs and even crash the process in newer versions.
• Unpredictable program flow: Without await-ing a promise, the execution of code could continue before the promise is resolved or rejected. This can lead to race conditions and other timing-related bugs, because it makes the order of execution unpredictable. [For our use case this also relates to the persistSession as noted below, socketio/socket.io#4697]
• ...

In this adapter it seems mostly to originate from the publish method, which inserts a record in the mongo collection. But almost none of the calls to publish are actually awaited.

More upstream, also other methods such as persistSession, onEvent, broadcastWithAck ... are not exposing the return value as promise. This might be caused as they have to implement a certain interface. But I think it would greatly improve the (semantic) readability and stability of the code base.

---

I understand that updating this in the entire codebase is a significant task, and that's why I'm asking: Would you accept a pull request that aims to resolve these issues? I'd be happy to contribute to improving this aspect of the codebase. We could start by just updating this connector.

[darrachequesne]

Sure, please open a pull request :+1:

[darrachequesne]

@joelluijmes the promise rejections should now be caught: https://github.com/socketio/socket.io-mongo-adapter/commit/075216f7decac3e8660c39dc1009a27d786ca1ad

I'm open to suggestions if you see any other improvement.

[joelluijmes]

I appreciate your prompt response and the effort you've put into addressing the issue. However, I'd like to clarify my concerns further.

While logging errors is a step in the right direction for debugging purposes, it doesn't truly handle the issues related to unhandled promises. Handling promises by returning them to the caller is a more robust approach. Let me emphasize my earlier points:

1. Silent Failures: Logging errors, although helpful for those with debug logging enabled, doesn't notify users about potential issues when they occur. Returning promises to the caller would allow them to handle errors appropriately based on their specific use case.
2. Unpredictable Program Flow: The timing-related issues and potential race conditions caused by unhandled promises still remain with the current approach. Returning promises ensures that the program flow remains predictable and controlled.

I'm genuinely interested in contributing to improving the codebase, and my proposal to address these issues is to refactor the interface so that promises are returned to the caller. This would enhance both the semantic readability and stability of the codebase.

However, I must admit that I have some reservations based on previous responses to similar suggestions. If there's a willingness to consider and merge such changes, I'd be more than happy to work on a pull request. My aim is to improve this aspect of the project, and if we can reach a consensus on the approach, it would benefit all users. Otherwise, we might consider maintaining our own fork to meet our specific needs.

[darrachequesne]

my proposal to address these issues is to refactor the interface so that promises are returned to the caller.

That's an interesting idea. Do you have a specific API in mind? For example, should io.emit() (which call adapter.broadcast() under the hood) return a promise?

However, I must admit that I have some reservations based on previous responses to similar suggestions.

Let's discuss about the details of the change first, if that's OK for you.

If you are referring to https://github.com/socketio/socket.io-mongo-adapter/pull/19, you were suggesting a change which would bring no tangible benefit, besides "this library was updated more recently" which is not really convincing (especially in the Node.js world). Please be sure that any change that brings actual benefits will be warmly welcomed :+1:

[joelluijmes]

That's an interesting idea. Do you have a specific API in mind? For example, should io.emit() (which call adapter.broadcast() under the hood) return a promise?

Well, all functions that come down to promises should propagate that promise back to the caller. I don't see why we should pick and choose specific methods. Perhaps this might imply that the API always return promise, where it might be resolved already when the code path is synchronous. [Note: at this point, I'm not sure yet how/if the base socket.io adapter should be modified.]

If you are referring to #19, you were suggesting a change which would bring no tangible benefit, besides "this library was updated more recently" which is not really convincing (especially in the Node.js world). Please be sure that any change that brings actual benefits will be warmly welcomed 👍

Honestly, I find it difficult to understand why a completed PR that removes deprecated packages wouldn't be considered a valuable contribution in an open-source project. However, I'd rather not engage in a debate where our fundamental views on how software should be developed and maintained seem to differ. So let's agree to disagree here.