Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Delete circular buffer zero-length file #192

Open
wants to merge 10 commits into
base: dev
Choose a base branch
from

Conversation

MV10
Copy link
Collaborator

@MV10 MV10 commented Oct 23, 2020

Hi Ian, this a quick-fix to remove the zero-length file that the circular buffer leaves behind after stream disposal. It also handles the edge case of disposal while recording is active (meaning it won't delete the valid in-progress file), including the scenario where StopRecording was called but Split was not (which can be detected if CurrentStream is null).

So I think it's pretty safe, I can't think of any other oddball scenarios.

I also tweaked the summaries of a couple file stream handler props/methods, I was having trouble remembering exactly which parts of the full pathname they return (some names seem a bit inconsistent). That wasn't strictly necessary of course, but my first attempt put the file deletion in that class -- until I realized video capture handler would delete its valid file (oops).

@MV10
Copy link
Collaborator Author

MV10 commented Oct 23, 2020

Hold off on this one while I re-think it tomorrow.

I've found that the zero-length file is not deleted if you never start recording from the buffer.

I'm not sure there is enough information to decide when it's safe to delete the file under all scenarios:

  • recording was never started (delete file)
  • recording was started, stopped, and split (delete file)
  • recording was started, stopped, and never split (do not delete)
  • recording was started and never stopped (do not delete)

@MV10
Copy link
Collaborator Author

MV10 commented Oct 23, 2020

Turns out the easiest thing to do is add a flag that explicitly tracks whether data has been written to the file. The only time it's zero-length is after the constructors or immediately after calling Split. Passes all my tests now!

@techyian
Copy link
Owner

Hi Jon, thanks for raising this. Does this issue only affect the Circular Buffer handler or is it worth implementing this change further up at the FileStreamCaptureHandler?

@MV10
Copy link
Collaborator Author

MV10 commented Oct 24, 2020

Good call, I think you're right. No current handler has the problem except the circular buffer -- the video handler immediately writes to the file -- but I suppose some future handler might exhibit the same problem.

I'm about to leave the house but I will revise the PR tomorrow morning.

@MV10
Copy link
Collaborator Author

MV10 commented Oct 25, 2020

Ok, fixed, thanks for the insight. I suppose even the video handler could exhibit the problem in an edge case where the app was shutting down just as the constructor or split was called.

I also removed a left-over MotionType prop in the video handler that I missed in the earlier motion detection PRs.

I'm not personally a fan of unit tests , but it occurs to me that this is probably something that is very testable. (I understand the attraction but I've been involved with too many mega-scale enterprise projects where deciphering test code and fixing test bugs is as much work as the real codebase.) Thanks to my personal prejudice against them, my test-writing-fu is rusty at best, and perhaps the worst unit test experience is when someone writes a block of tests that doesn't match the project's general testing style, if you know what I mean. So, apologies in advance for not handling that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants