Wizcorp / event-wrapper

Temporarily wrap around your EventEmitters
0 stars 1 forks source link

event-wrapper should not throw when "done" is called twice #2

Open kubicle opened 6 years ago

kubicle commented 6 years ago

I would like event-wrapper to silently ignore when done is called more than once.

In the same way as:

\<philosophical bit>

Some will say "Be happy when it finds a possible issue for you", and it is maybe sometimes the case.

However, I think it is usually an OK practice to design cleanup code so it can run more than once (and sometimes even when the setup code has not run yet). It results in simpler, smoother code for terminating your components or application, especially when several ways to terminate must co-exist, and when async events make things already complicated enough.

I could not find proper discussion on this on the web at this point, but it must exist.

\</philosophical bit>

kubicle commented 6 years ago

One example of a PR that introduced an occurence of this "unwanted" throw: https://github.com/Wizcorp/Dofus/pull/3452

The added code looked completely innocuous... If we wanted to catch this problem back then, or similar ones in the future, we need to keep extra state outside event-wrapper, simply to remember if we called "done" already... a bit like you would need to keep extra state next to your EventEmitter to remember if you removed your listeners already (if EventEmitter was "throwing" too).

ronkorving commented 6 years ago

I'm fine with the change 👍 Wanna make a PR?

SwiTool commented 5 years ago

Adding a warning/debug can just be fine, it helps sometimes when you program like a donkey and don't know why you get disconnected for (apparently) no reason, but then you notice that you sent 300 packets per sec, making you feel guilty for the bandwidth 😞