{"thread":{"id":"30909","subject":"[PATCH v2 1/1] index-pack: Disable threading on cygwin","startedAt":"2012-06-26T18:19:32Z","lastAt":"2012-06-26T22:57:26Z","messageCount":5,"participants":["Ramsay Jones","Junio C Hamano","Erik Faye-Lund"],"isPatch":true,"patchVersion":2,"patchTotal":1},"messages":[{"id":"194301","messageId":"4FE9FD34.5020406@ramsay1.demon.co.uk","threadId":"30909","inReplyTo":null,"subject":"[PATCH v2 1/1] index-pack: Disable threading on cygwin","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-06-26T18:19:32Z","receivedAt":"2012-06-26T18:19:32Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"From: Junio C Hamano \n\nThe Cygwin implementation of pread() is not thread-safe since, just\nlike the emulation provided by compat/pread.c, it uses a sequence of\nseek-read-seek calls. In order to avoid failues due to thread-safety\nissues, commit b038a61 disables threading when NO_PREAD is defined.\n(ie when using the emulation code in compat/pread.c).\n\nWe introduce a new build variable, NO_THREAD_SAFE_PREAD, which allows\nuse to disable the threaded index-pack code on cygwin, in addition to\nthe above NO_PREAD case.\n\nSigned-off-by: Ramsay Jones \n---\n\nHi Junio,\n\nI made some small changes to your patch:\n\n - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD\n - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather\n than in git-compat-util.h\n - set NO_THREAD_SAFE_PREAD in the non-conditional part of the\n cygwin config section (ie not just versions before 1.7.x)\n\nI can only test this by using it (all relevant tests pass with or without\nthis patch, after all), so I have installed it for day-to-day use. I don't\nanticipate any problems, but I guess this patch has not actually been\ntested yet ... :-D\n\nHTH\n\nATB,\nRamsay Jones\n\n Makefile | 8 ++++++++\n builtin/index-pack.c | 4 ++--\n 2 files changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 4592f1f..67d761e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -158,6 +158,9 @@ all::\n # Define NO_PREAD if you have a problem with pread() system call (e.g.\n # cygwin1.dll before v1.5.22).\n #\n+# Define NO_THREAD_SAFE_PREAD if your pread() implementation is not\n+# thread-safe. (e.g. compat/pread.c or cygwin)\n+#\n # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n # generally faster on your platform than accessing the working directory.\n #\n@@ -1051,6 +1054,7 @@ ifeq ($(uname_O),Cygwin)\n \t\tNO_IPV6 = YesPlease\n \t\tOLD_ICONV = UnfortunatelyYes\n \tendif\n+\tNO_THREAD_SAFE_PREAD = YesPlease\n \tNEEDS_LIBICONV = YesPlease\n \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n \tNO_TRUSTABLE_FILEMODE = UnfortunatelyYes\n@@ -1659,6 +1663,10 @@ endif\n ifdef NO_PREAD\n \tCOMPAT_CFLAGS += -DNO_PREAD\n \tCOMPAT_OBJS += compat/pread.o\n+\tNO_THREAD_SAFE_PREAD = YesPlease\n+endif\n+ifdef NO_THREAD_SAFE_PREAD\n+\tBASIC_CFLAGS += -DNO_THREAD_SAFE_PREAD\n endif\n ifdef NO_FAST_WORKING_DIRECTORY\n \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex dc2cfe6..4705478 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -39,8 +39,8 @@ struct base_data {\n \tint ofs_first, ofs_last;\n };\n \n-#if !defined(NO_PTHREADS) && defined(NO_PREAD)\n-/* NO_PREAD uses compat/pread.c, which is not thread-safe. Disable threading. */\n+#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)\n+/* pread() emulation is not thread-safe. Disable threading. */\n #define NO_PTHREADS\n #endif\n \n-- \n1.7.11.1.gef8c760\n"},{"id":"194308","messageId":"7vk3ytc0es.fsf@alter.siamese.dyndns.org","threadId":"30909","inReplyTo":"4FE9FD34.5020406@ramsay1.demon.co.uk","subject":"Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-26T19:56:27Z","receivedAt":"2012-06-26T19:56:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD\n\nSensible.\n\n> - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather\n> than in git-compat-util.h\n\nI think it is a bad change. When compat/ pread gets improved to be\nthread-safe, this will surely be missed.\n\n> - set NO_THREAD_SAFE_PREAD in the non-conditional part of the\n> cygwin config section (ie not just versions before 1.7.x)\n\nSorry that I missed it, and thanks for fixing it.\n\n> I can only test this by using it (all relevant tests pass with or without\n> this patch, after all), so I have installed it for day-to-day use. I don't\n> anticipate any problems, but I guess this patch has not actually been\n> tested yet ... :-D\n>\n> HTH\n>\n> ATB,\n> Ramsay Jones\n>\n> Makefile | 8 ++++++++\n> builtin/index-pack.c | 4 ++--\n> 2 files changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/Makefile b/Makefile\n> index 4592f1f..67d761e 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -158,6 +158,9 @@ all::\n> # Define NO_PREAD if you have a problem with pread() system call (e.g.\n> # cygwin1.dll before v1.5.22).\n> #\n> +# Define NO_THREAD_SAFE_PREAD if your pread() implementation is not\n> +# thread-safe. (e.g. compat/pread.c or cygwin)\n> +#\n> # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n> # generally faster on your platform than accessing the working directory.\n> #\n> @@ -1051,6 +1054,7 @@ ifeq ($(uname_O),Cygwin)\n> \t\tNO_IPV6 = YesPlease\n> \t\tOLD_ICONV = UnfortunatelyYes\n> \tendif\n> +\tNO_THREAD_SAFE_PREAD = YesPlease\n> \tNEEDS_LIBICONV = YesPlease\n> \tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n> \tNO_TRUSTABLE_FILEMODE = UnfortunatelyYes\n> @@ -1659,6 +1663,10 @@ endif\n> ifdef NO_PREAD\n> \tCOMPAT_CFLAGS += -DNO_PREAD\n> \tCOMPAT_OBJS += compat/pread.o\n> +\tNO_THREAD_SAFE_PREAD = YesPlease\n> +endif\n> +ifdef NO_THREAD_SAFE_PREAD\n> +\tBASIC_CFLAGS += -DNO_THREAD_SAFE_PREAD\n> endif\n> ifdef NO_FAST_WORKING_DIRECTORY\n> \tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n> diff --git a/builtin/index-pack.c b/builtin/index-pack.c\n> index dc2cfe6..4705478 100644\n> --- a/builtin/index-pack.c\n> +++ b/builtin/index-pack.c\n> @@ -39,8 +39,8 @@ struct base_data {\n> \tint ofs_first, ofs_last;\n> };\n> \n> -#if !defined(NO_PTHREADS) && defined(NO_PREAD)\n> -/* NO_PREAD uses compat/pread.c, which is not thread-safe. Disable threading. */\n> +#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)\n> +/* pread() emulation is not thread-safe. Disable threading. */\n> #define NO_PTHREADS\n> #endif\n"},{"id":"194312","messageId":"CABPQNSZ7=kqSjXPLior+LXqAt4AzcybOjJ1P8ZLQ+J-9gCgvdw@mail.gmail.com","threadId":"30909","inReplyTo":"7vk3ytc0es.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-06-26T21:00:59Z","receivedAt":"2012-06-26T21:00:59Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano wrote:\n> Ramsay Jones writes:\n>\n>>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD\n>\n> Sensible.\n>\n>>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather\n>>      than in git-compat-util.h\n>\n> I think it is a bad change.  When compat/ pread gets improved to be\n> thread-safe, this will surely be missed.\n\nBut CAN it be fixed? I don't think it could, at least not without\nwrapping ALL calls to functions that perform IO on file handles or\nfile descriptors...\n"},{"id":"194326","messageId":"7v62adaee3.fsf@alter.siamese.dyndns.org","threadId":"30909","inReplyTo":"CABPQNSZ7=kqSjXPLior+LXqAt4AzcybOjJ1P8ZLQ+J-9gCgvdw@mail.gmail.com","subject":"Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-26T22:37:24Z","receivedAt":"2012-06-26T22:37:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano wrote:\n>> Ramsay Jones writes:\n>>\n>>> - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD\n>>\n>> Sensible.\n>>\n>>> - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather\n>>> than in git-compat-util.h\n>>\n>> I think it is a bad change. When compat/ pread gets improved to be\n>> thread-safe, this will surely be missed.\n>\n> But CAN it be fixed? I don't think it could, at least not without\n> wrapping ALL calls to functions that perform IO on file handles or\n> file descriptors...\n\nIs that relevant? It may be true that both Erik and Junio are not\nbeing clever enough to come up with a solution offhand. But is that\na good justification to go against a sound engineering practice?\n\nThere may be more than one platforms that want their own different\npread emulations, some safe with thread and some others not. I\nthink you would prefer to see something like this:\n\n\t#if NO_PREAD\n #if on platform A\n\t# define NO_THREAD_SAFE_PREAD\n # define pread(fd, buf, count, ofs) git_pread_A((fd), (buf), (count), (ofs))\n\t#endif\n #if on platform B\n # define pread(fd, buf, count, ofs) git_pread_B((fd), (buf), (count), (ofs))\n\t#endif\n\nand keep \"make -DNO_PREAD\" to mean \"The platform does not have a\npread, please emulate\" and nothing else. The implementation of the\nemulation itself would be the one that knows of its thread safeness.\n\nHaving said that, I do not care too deeply either way, so the patch\nis queued as-is.\n"},{"id":"194330","messageId":"CABPQNSaqxouhpsHBN5uVCiaRpkuR65uNNn6c30CxEx3zGbJrSg@mail.gmail.com","threadId":"30909","inReplyTo":"7v62adaee3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-06-26T22:57:26Z","receivedAt":"2012-06-26T22:57:26Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Jun 27, 2012 at 12:37 AM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>> On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano wrote:\n>>> Ramsay Jones writes:\n>>>\n>>>>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD\n>>>\n>>> Sensible.\n>>>\n>>>>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather\n>>>>      than in git-compat-util.h\n>>>\n>>> I think it is a bad change.  When compat/ pread gets improved to be\n>>> thread-safe, this will surely be missed.\n>>\n>> But CAN it be fixed? I don't think it could, at least not without\n>> wrapping ALL calls to functions that perform IO on file handles or\n>> file descriptors...\n>\n> Is that relevant?  It may be true that both Erik and Junio are not\n> being clever enough to come up with a solution offhand.  But is that\n> a good justification to go against a sound engineering practice?\n>\n\nI didn't really mean to argue for or against this particular solution,\nsorry if I was unclear. I'm more interested in actually fixing the\nissue without disabling threading ;)\n\nAt least on Windows, the CRT actually does take a lock for the\nfile-handle for all standard IO opterations. Perhaps we can somehow\ntake it ourselves? I'm not entirely sure how to get hold of it, but it\nwould probably require quite a deep-dive into the CRT sources...\n"}]}