Fix tests and remove workaround#465
Conversation
Problem: Recently there was a PR [0] merged with a quickfix to avoid some test failures, which is something I've been trying to avoid. While `process.exit()` works fine, I'm worried that it means we don't understand what's happening under the hood, *plus* I have the [maybe unjustified?] worry that it might kill the process during a database write or something dangerous. It looks like this particular test hang was caused by both a stream and some number of intervals that were left open. Solution: Provide a way to close the stream and intervals in `index.js` and ensure that we do that before closing the server. [0]: fraction#462
| // one interval running at a time. | ||
| _startNameWarmup() { | ||
| const abortable = pullAbortable(); | ||
| let intervals = []; |
There was a problem hiding this comment.
I think the only thing I don't understand about this patch is this change, to have a list of intervals...
There was a problem hiding this comment.
Potential problem: I didn't see any code that ensures that we only have one interval running at a time. I think that the 'sync' events happen every time we're up-to-date, which gives me the impression that we might have multiple intervals active at once? I don't know, didn't have time to test but also really didn't want to miss any of these things.
Happy to merge an improvement removing the list if we can be sure there's only one interval active at a time.
There was a problem hiding this comment.
okay so after looking at this a bit harder.... I think I see why you made this into a list of intervals. you're expecting msg.sync to be true more than once?
If that's the case, then that should have already been the case before, no?
What I don't see in your code though is something that would ever remove an interval id from the list again.
Shouldn't the code that's triggered by the interval remove it from the list again?
There was a problem hiding this comment.
If that's the case, then that should have already been the case before, no?
Yep, exactly. Previously this interval wasn't being cleared, which is one reason why the tests hung forever -- this clears the intervals, but since I'm not sure how many there are then I'm double-checking to make sure there can't be more than one.
(This could also be accomplished with an if.)
What I don't see in your code though is something that would ever remove an interval id from the list again.
Like this, or do you mean something else?
There was a problem hiding this comment.
That clears the interval, yes. But the integer (?) referencing this interval will still be held in the intervals variable, no?
There was a problem hiding this comment.
(sorry if I'm wasting your time. I haven't used these before, not really, and I'm just going by what I find in MDN)
There was a problem hiding this comment.
That's right! But this only happens when the Oasis closes so we don't need to worry about removing items from memory. If we were clearing items every few minutes or so, you're absolutely correct that this would cause a memory leak. I'd be happy to merge a PR improving this to do it a different way, I mostly just want to fix up the tests.
There was a problem hiding this comment.
No, just checking that I understand what's happening. Clearing up a few bytes just before exit seems... non-prioritary :P
|
Self-merging. |
2 participants
What's the problem you solved?
Problem: Recently there was a PR 0 merged with a quickfix to avoid
some test failures, which is something I've been trying to avoid. While
process.exit()works fine, I'm worried that it means we don'tunderstand what's happening under the hood, plus I have the [maybe
unjustified?] worry that it might kill the process during a database
write or something dangerous. It looks like this particular test hang
was caused by both a stream and some number of intervals that were left
open.
What solution are you recommending?
Solution: Provide a way to close the stream and intervals in
index.jsand ensure that we do that before closing the server.
cc: @cryptix @cblgh @black-puppydog @cinnamon-bun