{"thread":{"id":"36013","subject":"[GSoC14][RFC] Proposal Draft: Refactor tempfile handling","startedAt":"2014-03-01T21:04:39Z","lastAt":"2014-03-12T12:26:21Z","messageCount":7,"participants":["Brian Gesiak","Jeff King","Michael Haggerty"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"235761","messageId":"CAN7MxmVQuk96dmXfxZ5kRZPTXNwpz2RY=y8HyqX4mZzrZUVbNg@mail.gmail.com","threadId":"36013","inReplyTo":null,"subject":"[GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-03-01T21:04:39Z","receivedAt":"2014-03-01T21:04:39Z","isPatch":false,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"Hello all,\n\nMy name is Brian Gesiak. I'm a research student at the University of\nTokyo, and I'm hoping to participate in this year's Google Summer of\nCode by contributing to Git. I'm a longtime user, first-time\ncontributor--some of you may have noticed my \"microproject\"\npatches.[1][2]\n\nI'd like to gather some information on one of the GSoC ideas posted on\nthe ideas page. Namely, I'm interested in refactoring the way\ntempfiles are cleaned up.\n\nThe ideas page points out that while lock files are closed and\nunlinked[3] when the program exits[4], object pack files implement\ntheir own brand of temp file creation and deletion. This\nimplementation doesn't share the same guarantees as lock files--it is\npossible that the program terminates before the temp file is\nunlinked.[5]\n\nLock file references are stored in a linked list. When the program\nexits, this list is traversed and each file is closed and unlinked. It\nseems to me that this mechanism is appropriate for temp files in\ngeneral, not just lock files. Thus, my proposal would be to extract\nthis logic into a separate module--tempfile.h, perhaps. Lock and\nobject files would share the tempfile implementation.\n\nThat is, both object and lock temp files would be stored in a linked\nlist, and all of these would be deleted at program exit.\n\nI'm very enthused about this project--I think it has it all:\n\n- Tangible benefits for the end-user\n- Reduced complexity in the codebase\n- Ambitious enough to be interesting\n- Small enough to realistically be completed in a summer\n\nPlease let me know if this seems like it would make for an interesting\nproposal, or if perhaps there is something I am overlooking. Any\nfeedback at all would be appreciated. Thank you!\n\n- Brian Gesiak\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/242891\n[2] http://thread.gmane.org/gmane.comp.version-control.git/242893\n[3] https://github.com/git/git/blob/v1.9.0/lockfile.c#L18\n[4] https://github.com/git/git/blob/v1.9.0/lockfile.c#L143\n[5] https://github.com/git/git/blob/v1.9.0/pack-write.c#L350\n"},{"id":"235944","messageId":"20140303224238.GA2699@sigill.intra.peff.net","threadId":"36013","inReplyTo":"CAN7MxmVQuk96dmXfxZ5kRZPTXNwpz2RY=y8HyqX4mZzrZUVbNg@mail.gmail.com","subject":"Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-03T22:42:38Z","receivedAt":"2014-03-03T22:42:38Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 02, 2014 at 06:04:39AM +0900, Brian Gesiak wrote:\n\n> My name is Brian Gesiak. I'm a research student at the University of\n> Tokyo, and I'm hoping to participate in this year's Google Summer of\n> Code by contributing to Git. I'm a longtime user, first-time\n> contributor--some of you may have noticed my \"microproject\"\n> patches.[1][2]\n\nYes, we did notice them. Thanks, and welcome. :)\n\n> The ideas page points out that while lock files are closed and\n> unlinked[3] when the program exits[4], object pack files implement\n> their own brand of temp file creation and deletion. This\n> implementation doesn't share the same guarantees as lock files--it is\n> possible that the program terminates before the temp file is\n> unlinked.[5]\n> \n> Lock file references are stored in a linked list. When the program\n> exits, this list is traversed and each file is closed and unlinked. It\n> seems to me that this mechanism is appropriate for temp files in\n> general, not just lock files. Thus, my proposal would be to extract\n> this logic into a separate module--tempfile.h, perhaps. Lock and\n> object files would share the tempfile implementation.\n> \n> That is, both object and lock temp files would be stored in a linked\n> list, and all of these would be deleted at program exit.\n\nYes, I think this is definitely the right way to go. We should be able\nto unify the tempfile handling for all of git. Once the logic is\nextracted into a nice API, there are several other places that can use\nit, too:\n\n  - the external diff code creates tempfiles and uses its own cleanup\n    routines\n\n  - the shallow_XXXXXX tempfiles (these are not cleaned right now,\n    though I sent a patch recently for them to do their own cleanup)\n\nThose are just off the top of my head. There may be other spots, too.\n\nIt is worth thinking in your proposal about some of the things that the\nAPI will want to handle. What are the mismatches in how lockfiles and\nobject files are handled? E.g., how do we finalize them into place?\nHow should the API be designed to minimize race conditions (e.g., if we\nget a signal delivered while we are committing or cleaning up a file)?\n\n> Please let me know if this seems like it would make for an interesting\n> proposal, or if perhaps there is something I am overlooking. Any\n> feedback at all would be appreciated. Thank you!\n\nYou definitely have a grasp of what the project is aiming for, and which\nareas need to be touched.\n\n-Peff\n"},{"id":"236322","messageId":"CAN7MxmW-aWgTQpTMuEx=kzyHVUf5E7unZR-LmLQrY-AmmrZxjA@mail.gmail.com","threadId":"36013","inReplyTo":"20140303224238.GA2699@sigill.intra.peff.net","subject":"Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-03-08T17:04:16Z","receivedAt":"2014-03-08T17:04:16Z","isPatch":false,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"Excellent, thank you very much for the feedback, Jeff! It was very\nhelpful and encouraging. I've done some more research based on your\ncomments.\n\n> Once the logic is extracted into a nice API, there are\n> several other places that can use it, too: ...\n\nI've found the following four areas so far:\n\n1. lockfile.lock_file\n2. git-compat-util.odb_mkstemp\n3. git-compat-util.odb_pack_keep\n4. diff.prepare_temp_file\n\nTons of files use (1) and (2). (3) is less common, and (4) is only\nused for external diffs.\n\n> the shallow_XXXXXX tempfiles\n\nI'm not sure I was able to find this one. Are you referring to the\nlock files used when fetching, such as in fetch-pack.c?\n\n> What are the mismatches in how lockfiles and object files are\n> handled? E.g., how do we finalize them into place? How should\n> the API be designed to minimize race conditions (e.g., if we\n> get a signal delivered while we are committing or cleaning up a file)?\n\nI'd say the biggest difference between lockfiles and object files is\nthat tempfile methods like odb_mkstemp need to know the location of\nthe object directory. Aside from that, lockfiles and the external diff\nfiles appear to be cleaned up at exit, while temporary object files\ntend to have a more finely controlled lifecycle. I'm still\ninvestigating this aspect of the proposal, though.\n\nOne question, though: the idea on the ideas page specifies that\ntemporary pack and object files may \"optionally\" be cleaned up in case\nof error during program execution. How will users specify their\npreference? I think the API for creating temporary files should allow\ncleanup options to be specified on a per-file basis. That way each\npart of the program that creates tempfiles can specify a different\nconfig value to determine the cleanup policy.\n\nThanks for all your help so far!\n\n- Brian Gesiak\n\nPS: I'm maintaining a working draft of my proposal here, in case\nanyone wants to offer any feedback prior to its submission:\nhttps://gist.github.com/modocache/9434914\n\n\nOn Tue, Mar 4, 2014 at 7:42 AM, Jeff King <peff@peff.net> wrote:\n> On Sun, Mar 02, 2014 at 06:04:39AM +0900, Brian Gesiak wrote:\n>\n>> My name is Brian Gesiak. I'm a research student at the University of\n>> Tokyo, and I'm hoping to participate in this year's Google Summer of\n>> Code by contributing to Git. I'm a longtime user, first-time\n>> contributor--some of you may have noticed my \"microproject\"\n>> patches.[1][2]\n>\n> Yes, we did notice them. Thanks, and welcome. :)\n>\n>> The ideas page points out that while lock files are closed and\n>> unlinked[3] when the program exits[4], object pack files implement\n>> their own brand of temp file creation and deletion. This\n>> implementation doesn't share the same guarantees as lock files--it is\n>> possible that the program terminates before the temp file is\n>> unlinked.[5]\n>>\n>> Lock file references are stored in a linked list. When the program\n>> exits, this list is traversed and each file is closed and unlinked. It\n>> seems to me that this mechanism is appropriate for temp files in\n>> general, not just lock files. Thus, my proposal would be to extract\n>> this logic into a separate module--tempfile.h, perhaps. Lock and\n>> object files would share the tempfile implementation.\n>>\n>> That is, both object and lock temp files would be stored in a linked\n>> list, and all of these would be deleted at program exit.\n>\n> Yes, I think this is definitely the right way to go. We should be able\n> to unify the tempfile handling for all of git. Once the logic is\n> extracted into a nice API, there are several other places that can use\n> it, too:\n>\n>   - the external diff code creates tempfiles and uses its own cleanup\n>     routines\n>\n>   - the shallow_XXXXXX tempfiles (these are not cleaned right now,\n>     though I sent a patch recently for them to do their own cleanup)\n>\n> Those are just off the top of my head. There may be other spots, too.\n>\n> It is worth thinking in your proposal about some of the things that the\n> API will want to handle. What are the mismatches in how lockfiles and\n> object files are handled? E.g., how do we finalize them into place?\n> How should the API be designed to minimize race conditions (e.g., if we\n> get a signal delivered while we are committing or cleaning up a file)?\n>\n>> Please let me know if this seems like it would make for an interesting\n>> proposal, or if perhaps there is something I am overlooking. Any\n>> feedback at all would be appreciated. Thank you!\n>\n> You definitely have a grasp of what the project is aiming for, and which\n> areas need to be touched.\n>\n> -Peff\n"},{"id":"236474","messageId":"20140311013316.GA11524@sigill.intra.peff.net","threadId":"36013","inReplyTo":"CAN7MxmW-aWgTQpTMuEx=kzyHVUf5E7unZR-LmLQrY-AmmrZxjA@mail.gmail.com","subject":"Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-11T01:33:17Z","receivedAt":"2014-03-11T01:33:17Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 09, 2014 at 02:04:16AM +0900, Brian Gesiak wrote:\n\n> > Once the logic is extracted into a nice API, there are\n> > several other places that can use it, too: ...\n> \n> I've found the following four areas so far:\n> \n> 1. lockfile.lock_file\n> 2. git-compat-util.odb_mkstemp\n> 3. git-compat-util.odb_pack_keep\n> 4. diff.prepare_temp_file\n> \n> Tons of files use (1) and (2). (3) is less common, and (4) is only\n> used for external diffs.\n\nYeah, I would expect (1) and (2) to be the most frequent. (3) gets\nwritten on every push and fetch, but only for a short period. (4) is\nalso used for diff's textconv, though like external diffs, they are\nrelatively rare.\n\nIn my experience, most of the cruft that gets left is from (2), since a\npush or fetch will spool to a tmpfile, then verify the results via \"git\nindex-pack\". Any failure there leaves the file in place.\n\nThere are a few other potential candidates we can find by grepping for\nmkstemp. Not all of those might want cleanup, but it's a starting point\nfor investigation.\n\n> > the shallow_XXXXXX tempfiles\n> \n> I'm not sure I was able to find this one. Are you referring to the\n> lock files used when fetching, such as in fetch-pack.c?\n\nI mean the xmkstemp from setup_temporary_shallow in shallow.c.\n\n> I'd say the biggest difference between lockfiles and object files is\n> that tempfile methods like odb_mkstemp need to know the location of\n> the object directory. Aside from that, lockfiles and the external diff\n> files appear to be cleaned up at exit, while temporary object files\n> tend to have a more finely controlled lifecycle. I'm still\n> investigating this aspect of the proposal, though.\n\nThe diff tempfiles are true tempfiles; they always go away in the end\n(though of course we want to clean them up as we finish with them,\nrather than doing it all at the end). Lockfiles may get committed into\nplace (i.e., via atomic rename) or rolled back (deleted).\n\nObject files should generally be hard-linked into place, but there is\nsome extra magic in move_temp_to_file to fallback to renames.  Some of\nthat we may be able to get rid of (e.g., we try to avoid doing\ncross-directory renames at all these days, so the comment there may be\nout of date).\n\n> One question, though: the idea on the ideas page specifies that\n> temporary pack and object files may \"optionally\" be cleaned up in case\n> of error during program execution. How will users specify their\n> preference? I think the API for creating temporary files should allow\n> cleanup options to be specified on a per-file basis. That way each\n> part of the program that creates tempfiles can specify a different\n> config value to determine the cleanup policy.\n\nThat probably makes sense. I certainly had a config option in mind. I\nmentioned above that the most common cruft is leftover packfiles from\npushes and fetches. We haven't deleted those historically because the\nsame person often controls both the client and the server, and they\nwould want to possibly do forensics on the packfile sent to the remote,\nor even rescue objects out of it. But the remote end may simply have\nrejected the pack by some policy, and has no interest in forensics.\n\nHaving a config option for each type of file may be cool, but I don't\nknow how useful it would be in practice. Still, it's certainly worth\nthinking about and looking into.\n\n-Peff\n"},{"id":"236508","messageId":"531F3959.4060608@alum.mit.edu","threadId":"36013","inReplyTo":"CAN7MxmVQuk96dmXfxZ5kRZPTXNwpz2RY=y8HyqX4mZzrZUVbNg@mail.gmail.com","subject":"Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-11T16:27:05Z","receivedAt":"2014-03-11T16:27:05Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/01/2014 10:04 PM, Brian Gesiak wrote:\n> Hello all,\n> \n> My name is Brian Gesiak. I'm a research student at the University of\n> Tokyo, and I'm hoping to participate in this year's Google Summer of\n> Code by contributing to Git. I'm a longtime user, first-time\n> contributor--some of you may have noticed my \"microproject\"\n> patches.[1][2]\n> \n> I'd like to gather some information on one of the GSoC ideas posted on\n> the ideas page. Namely, I'm interested in refactoring the way\n> tempfiles are cleaned up.\n> \n> The ideas page points out that while lock files are closed and\n> unlinked[3] when the program exits[4], object pack files implement\n> their own brand of temp file creation and deletion. This\n> implementation doesn't share the same guarantees as lock files--it is\n> possible that the program terminates before the temp file is\n> unlinked.[5]\n> \n> Lock file references are stored in a linked list. When the program\n> exits, this list is traversed and each file is closed and unlinked. It\n> seems to me that this mechanism is appropriate for temp files in\n> general, not just lock files. Thus, my proposal would be to extract\n> this logic into a separate module--tempfile.h, perhaps. Lock and\n> object files would share the tempfile implementation.\n> \n> That is, both object and lock temp files would be stored in a linked\n> list, and all of these would be deleted at program exit.\n> \n> I'm very enthused about this project--I think it has it all:\n> \n> - Tangible benefits for the end-user\n> - Reduced complexity in the codebase\n> - Ambitious enough to be interesting\n> - Small enough to realistically be completed in a summer\n> \n> Please let me know if this seems like it would make for an interesting\n> proposal, or if perhaps there is something I am overlooking. Any\n> feedback at all would be appreciated. Thank you!\n\nHi Brian,\n\nThanks for your proposal.  I have a technical point that I think your\nproposal should address:\n\nCurrently the linked list of lockfiles only grows, never shrinks.  Once\nan object has been linked into the list, there is no way to remove it\nagain even after the lock has been released.  So if a lock needs to be\ncreated dynamically at a random place in the code, its memory is\nunavoidably leaked.\n\nThis hasn't been much of a problem in the past because (1) the number of\nlocks acquired/released during a Git invocation is reasonable, and (2) a\nlock object (even if it is already in the list) can be reused after the\nlock has been released.  So there are many lock callsites that define\none static lock instance and use it over and over again.\n\nBut I have a feeling that if we want to use a similar mechanism to\nhandle all temporary files (of which there can be more), then it would\nbe a good idea to lift this limitation.  It will require some care,\nthough, to make sure that record removal is done in a way that is\nthreadsafe and safe in the event of all expected kinds of process death.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236514","messageId":"20140311180559.GB28154@sigill.intra.peff.net","threadId":"36013","inReplyTo":"531F3959.4060608@alum.mit.edu","subject":"Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-11T18:05:59Z","receivedAt":"2014-03-11T18:05:59Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 11, 2014 at 05:27:05PM +0100, Michael Haggerty wrote:\n\n> Thanks for your proposal.  I have a technical point that I think your\n> proposal should address:\n> \n> Currently the linked list of lockfiles only grows, never shrinks.  Once\n> an object has been linked into the list, there is no way to remove it\n> again even after the lock has been released.  So if a lock needs to be\n> created dynamically at a random place in the code, its memory is\n> unavoidably leaked.\n\nThanks, I remember thinking about this when I originally conceived of\nthe idea, but I forgot to mention it in the idea writeup.\n\nIn most cases the potential leaks are finite and small, but object\ncreation and diff tempfiles could both be unbounded. So this is\ndefinitely something to consider. In both cases we have a bounded number\nof _simultaneous_ tempfiles, so one strategy could be to continue using\nstatic objects. But it should not be hard to do it dynamically, and I\nsuspect the resulting API will be a lot easier to comprehend.\n\n-Peff\n"},{"id":"236573","messageId":"CAN7MxmXzowHjPFKS_+Dw8n8GUX7ooqTXW6MAGpNLA-VxnFY9uQ@mail.gmail.com","threadId":"36013","inReplyTo":"20140311180559.GB28154@sigill.intra.peff.net","subject":"Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling","fromName":"Brian Gesiak","fromEmail":"modocache@gmail.com","sentAt":"2014-03-12T12:26:21Z","receivedAt":"2014-03-12T12:26:21Z","isPatch":false,"sender":{"key":"modocache@gmail.com","avatar":"https://avatars.githubusercontent.com/u/552921?v=4"},"body":"> Currently the linked list of lockfiles only grows, never shrinks.  Once\n> an object has been linked into the list, there is no way to remove it\n> again even after the lock has been released.  So if a lock needs to be\n> created dynamically at a random place in the code, its memory is\n> unavoidably leaked.\n\nAh yes, I see. I think a good example is\nconfig.git_config_set_multivar_in_file, which even contains a comment\ndetailing the problem: \"Since lockfile.c keeps a linked list of all\ncreated lock_file structures, it isn't safe to free(lock).  It's\nbetter to just leave it hanging around.\"\n\n> But I have a feeling that if we want to use a similar mechanism to\n> handle all temporary files (of which there can be more), then it would\n> be a good idea to lift this limitation.  It will require some care,\n> though, to make sure that record removal is done in a way that is\n> threadsafe and safe in the event of all expected kinds of process death.\n\nIt sounds like a threadsafe linked-list with an interface to manually\nremove elements from the list is the solution here; does that sound\nreasonable? Ensuring thread safety without sacrificing readability is\nprobably more difficult than it sounds, but I don't think it's\nimpossible.\n\nI'll add some more details on this to my proposal[1]. Thank you!\n\n- Brian Gesiak\n\n[1] https://www.google-melange.com/gsoc/proposal/review/student/google/gsoc2014/modocache/5629499534213120\n"}]}