versatica
Deprecate `observer` properties
Feature Request
I understand the reasons of use the observer pattern, but as it's shown in the API docs, we have two different event emitters, the actual Mediasoup objects and their observers. I think the difference came from the C++ side, but in Javascript we don't have such limitation, so I propose to deprecate the observer properties, and make the actual Mediasoup objects to emit that events. We have already some of them emitted by both, so first we would make that the Mediasoup objects emit all the events and deprecate the observer property, and remove it in a future major version. This will clean the API and make it more Node.js alike.
I agree on this one, there were many cases where I used observer in real apps for state maintenance (to delete something from the map when it is closed for instance). Also Rust version doesn't have such distinction and it doesn't seem to cause any issues in practice.
Design of mediasoup API is that user actions (calling methods on mediasoup objects) do not produce events in those objects. This is not an uncommon approach. It doesn't make sense to get a "close" event in object xxx after you call xxx.close(), you already know you are closing it.
Observers have a different role. They are designed to allow external modules to monitor everything that happens in your mediasoup app without having to integrate them into every business logic of your app.
It doesn't make sense to get a "close" in object
xxxafter you callxxx.close(), you already know you are closing it.
Sort of agree on that. On the other hand, being most operations asincronous (close is the main exception, I would make it async too but that's another story) it can make sense to emit the event after calling the method to say "yes, NOW it's really closed". For example, WebSockets API works that way, you can .close() it, it enters a CLOSSING state, and when receives back the close confirmation, it enters the CLOSED state and emits the close event.
Observers have a different role. They are designed to allow external modules to monitor everything that happens in your mediasoup app without having to integrate them into every business logic of your app.
But, isn't this possible by dispatching the events in the actual objects? Must to admit that the observers have solved me a huge architecture flaw on Mafalda and also cleaned a lot of code and more that's to come, so good for them :-) Just curious why this would not be possible by emitting the events in the actual objects instead of the observers, specially since both are EventEmitter instances...
I hate CLOSING states and async close. They are a design failure. This is not debatable.
By listening to events in observers you know that those events are emitted no matter who triggered it. I know this is cosmetic.
Imagine parent checking socket state: "oh, it's closing so I'll behave different than if it was in closed state". Does it make sense? No. From parent point of view, being closing or closed is irrelevant. It's destroyed, it's over, it's gone. Period. Do not interact again with that object and remove all its references.
Close methods must be sync and idempotent. Once you call them the object should be closed, it should reject/throw if any other public method is called by parent, and it should not emit anything (but "close" in some cases if there is a parent listening for that event). If close() method needs to clean internal stuff and run async cleanup code it's implementation details. Close method should not be async.
Said that, I don't understand. All close methods in mediasoup are sync. They immediately close the object and clean their internal resources. Parent or app doesn't need to do stupid things waiting for a close method to resolve nor it needs to check whether state is "closed" or "closing". There is just a obj.closed flag which is set to true once close() method is called (by app or internally).
There are exactly zero issues with this design. May I know what's the rationale to suggest making close method async and introduce new "closing" state? Just because WebSocket API sucks we should do the same?
I almost totally agree in all your points, but there are some corner cases to consider:
From parent point of view, being closing or closed is irrelevant. It's destroyed, it's over, it's gone. Period. Do not interact again with that object and remove all its references.
Yes, but in this case CLOSING means it's in an unstable state, like yellow light in a traffic intersection. Should you deal with the object? No, it's going to be dead. But maybe you would be interested on doing whatever you need to do once it's totally dead. That's why I created https://github.com/Takeafile/promiseWebSocket, it returns a Promise that gets resolved if the WebSocket is open or rejects it if it's closed, and wait on OPENING and CLOSING until it's not "unstable" anymore. Personally I don't find that extra states bad, but instead I find them informative... In the same way, when doing network requests I use sometimes an "inFlight" state to flag a request is being send and we are waiting for an answer, since we don't know if it has been received or not, and there's a time hole while the request is traveling the network.
If close() method needs to clean internal stuff and run async cleanup code it's implementation details.
Ok, agree in that you should not use that object anymore. But if close operation fails, how do you notify it? How do you know the object is actually closed and all its internal state is cleaned? I personally prefer to have an error in the close operation that tells me that...
May I know what's the rationale to suggest making close method async and introduce new "closing" state?
Must to admit my rationale is weak and biased, but my reasons to propose that are:
- homogeneous API, all the objects other methods are asynchronous
- notify errors if closing went wrong
Close() methods should never fail, that's my point. That's why I said that close() must be idempotent. If they "fail" that's a bug in the library or app. And whatever error happens while closing should be handled internally by the object, it should not be exposed to the parent. I've never seen any real and useful use case in which parent needs to know whether an object is closed or still closing. I'd even say the same for open and opening, but I admit that it maybe a bit different depending on what the object does.
Now let's go to the real world. Have you identified any case in which any close() method in mediasoup fails or doesn't do what it's supposed to do? I've never seen that :)
I'll expose a different point of view:
- parent calls "await child.close()".
- it rejects due to some error.
- now what, should parent call close() again? maybe with more energy? or should it call some child.reallyDestroyPlease() method?
It doesn't make any sense.
Close() methods should never fail, that's my point. That's why I said that close() must be idempotent.
Agree. But in a networked environment, a remote close() can fail due to the network for example. The application is fine, but the operation can fail, so what to do here? Honest question, probably biased and not Mediasoup specific, but I've faced this situation before... Client can assume that since it called the close/destroy/stop/whatever, that it actually worked, but maybe it failed and the server has it still running...
Probably I'm mixing topics here, sorry for that.
- should parent call close() again? maybe with more energy? or should it call some child.reallyDestroyPlease() method?
It depends of each situation, but definitely, parent / user should be notified, error should not be silenced, and log it in a side channel is not a good solution because you lost the context where it was called.
Coming back to mediasoup, have you identified any case in which close() fails or doesn't do what it's supposed to do? :)
Coming back to mediasoup, have you identified any case in which close() fails or doesn't do what it's supposed to do? :)
Not at the moment. In any case, retaking the thread, this is about deprecate the observer property and make actual objects to emit all the events, not only close, specially since some of them are emitted by both the actual object and its observer, and they seems to be duplicated.
Let's close this ticket. Will consider it in v4.
Is v4 roadmap already available?
Nope and no ETA being honest.
Ok, thanks for clarify, please notify when you open it.