fix(server): don't wait for socket.io store expiration timeout #1040
Conversation
|
What is this expiration timeout for? I don't think Karma uses the store, so this should be fine. Can you add a comment, explaining why we are setting it to 0? |
|
@vojtajina I'm not super-familiar with socket.io internals, so I'm not sure what is the exact usage for this timeout. I presume that it might be used to persist data if a given client disconnects and re-connects but this is pure speculation. Anyway, here is the relevant code in socket.io: I will try to dig into this over the weekend and add a comment to this config change. |
|
This is why proper commit messages are important: socketio/socket.io@06421dd If the commit explained the change, it would be much simpler... Anyway, I dont' think we use the store (I know we don't use it directly), so I think it should be fine to set this to zero. Just add a comment to explain why. |
|
@vojtajina I've updated the commit to include more info / documentation in the code itself. Hopefully it is making clear what is going on here. |
|
Thanks @pkozlowski-opensource, merging... |
Hangs were fixed in 0.12.15/16 karma-runner/karma#1040 karma-runner/karma#1054
Judging by the discussion in PR karma-runner#1040, where this option was added, and the change in socket.io's Client.prototype.destroy function (which no longer includes a timeout), I feel pretty confident we can remove this option
This is kind of "question PR" (sorry, myself I hate those types of PRs but still too new to Karma internals to be able to say if this is the best approach or not...).
So, the story is the following one: socket.io has a timeout to clean data in a store, ex:
https://github.com/LearnBoost/socket.io/blob/0.9/lib/stores/memory.js#L137
Unfortunately this timeout prevents Karma's node process from existing properly when the test run is finished. The quickest fix is to default this timeout to 0. While it fixes my immediate problem, I'm not sure what are the consequences to Karma of doing so. I'm not sure if Karma is associating any data with a socket in the way that would make usage of socket.io store - in this case defaulting this timeout to 0 could have some side effect.
So, once again, fishing for feedback here more than expecting it to be merged immediately.