[PATCH v2 1/1] index-pack: Disable threading on cygwin

[PATCH v2 1/1] index-pack: Disable threading on cygwin

[Ramsay Jones]

From: Junio C Hamano <gitster@pobox.com>

The Cygwin implementation of pread() is not thread-safe since, just
like the emulation provided by compat/pread.c, it uses a sequence of
seek-read-seek calls. In order to avoid failues due to thread-safety
issues, commit b038a61 disables threading when NO_PREAD is defined.
(ie when using the emulation code in compat/pread.c).

We introduce a new build variable, NO_THREAD_SAFE_PREAD, which allows
use to disable the threaded index-pack code on cygwin, in addition to
the above NO_PREAD case.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Junio,

I made some small changes to your patch:

   - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD
   - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather
     than in git-compat-util.h
   - set NO_THREAD_SAFE_PREAD in the non-conditional part of the
     cygwin config section (ie not just versions before 1.7.x)

I can only test this by using it (all relevant tests pass with or without
this patch, after all), so I have installed it for day-to-day use. I don't
anticipate any problems, but I guess this patch has not actually been
tested yet ... :-D

HTH

ATB,
Ramsay Jones

 Makefile             | 8 ++++++++
 builtin/index-pack.c | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 4592f1f..67d761e 100644
--- a/Makefile
+++ b/Makefile
@@ -158,6 +158,9 @@ all::
 # Define NO_PREAD if you have a problem with pread() system call (e.g.
 # cygwin1.dll before v1.5.22).
 #
+# Define NO_THREAD_SAFE_PREAD if your pread() implementation is not
+# thread-safe. (e.g. compat/pread.c or cygwin)
+#
 # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
 # generally faster on your platform than accessing the working directory.
 #
@@ -1051,6 +1054,7 @@ ifeq ($(uname_O),Cygwin)
 		NO_IPV6 = YesPlease
 		OLD_ICONV = UnfortunatelyYes
 	endif
+	NO_THREAD_SAFE_PREAD = YesPlease
 	NEEDS_LIBICONV = YesPlease
 	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
 	NO_TRUSTABLE_FILEMODE = UnfortunatelyYes
@@ -1659,6 +1663,10 @@ endif
 ifdef NO_PREAD
 	COMPAT_CFLAGS += -DNO_PREAD
 	COMPAT_OBJS += compat/pread.o
+	NO_THREAD_SAFE_PREAD = YesPlease
+endif
+ifdef NO_THREAD_SAFE_PREAD
+	BASIC_CFLAGS += -DNO_THREAD_SAFE_PREAD
 endif
 ifdef NO_FAST_WORKING_DIRECTORY
 	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index dc2cfe6..4705478 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -39,8 +39,8 @@ struct base_data {
 	int ofs_first, ofs_last;
 };
 
-#if !defined(NO_PTHREADS) && defined(NO_PREAD)
-/* NO_PREAD uses compat/pread.c, which is not thread-safe. Disable threading. */
+#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
+/* pread() emulation is not thread-safe. Disable threading. */
 #define NO_PTHREADS
 #endif
 
-- 
1.7.11.1.gef8c760

Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD

Sensible.

>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather
>      than in git-compat-util.h

I think it is a bad change.  When compat/ pread gets improved to be
thread-safe, this will surely be missed.

>    - set NO_THREAD_SAFE_PREAD in the non-conditional part of the
>      cygwin config section (ie not just versions before 1.7.x)

Sorry that I missed it, and thanks for fixing it.

> I can only test this by using it (all relevant tests pass with or without
> this patch, after all), so I have installed it for day-to-day use. I don't
> anticipate any problems, but I guess this patch has not actually been
> tested yet ... :-D
>
> HTH
>
> ATB,
> Ramsay Jones
>
>  Makefile             | 8 ++++++++
>  builtin/index-pack.c | 4 ++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 4592f1f..67d761e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -158,6 +158,9 @@ all::
>  # Define NO_PREAD if you have a problem with pread() system call (e.g.
>  # cygwin1.dll before v1.5.22).
>  #
> +# Define NO_THREAD_SAFE_PREAD if your pread() implementation is not
> +# thread-safe. (e.g. compat/pread.c or cygwin)
> +#
>  # Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is
>  # generally faster on your platform than accessing the working directory.
>  #
> @@ -1051,6 +1054,7 @@ ifeq ($(uname_O),Cygwin)
>  		NO_IPV6 = YesPlease
>  		OLD_ICONV = UnfortunatelyYes
>  	endif
> +	NO_THREAD_SAFE_PREAD = YesPlease
>  	NEEDS_LIBICONV = YesPlease
>  	NO_FAST_WORKING_DIRECTORY = UnfortunatelyYes
>  	NO_TRUSTABLE_FILEMODE = UnfortunatelyYes
> @@ -1659,6 +1663,10 @@ endif
>  ifdef NO_PREAD
>  	COMPAT_CFLAGS += -DNO_PREAD
>  	COMPAT_OBJS += compat/pread.o
> +	NO_THREAD_SAFE_PREAD = YesPlease
> +endif
> +ifdef NO_THREAD_SAFE_PREAD
> +	BASIC_CFLAGS += -DNO_THREAD_SAFE_PREAD
>  endif
>  ifdef NO_FAST_WORKING_DIRECTORY
>  	BASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index dc2cfe6..4705478 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -39,8 +39,8 @@ struct base_data {
>  	int ofs_first, ofs_last;
>  };
>  
> -#if !defined(NO_PTHREADS) && defined(NO_PREAD)
> -/* NO_PREAD uses compat/pread.c, which is not thread-safe. Disable threading. */
> +#if !defined(NO_PTHREADS) && defined(NO_THREAD_SAFE_PREAD)
> +/* pread() emulation is not thread-safe. Disable threading. */
>  #define NO_PTHREADS
>  #endif

Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin

[Erik Faye-Lund]

On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>
>>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD
>
> Sensible.
>
>>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather
>>      than in git-compat-util.h
>
> I think it is a bad change.  When compat/ pread gets improved to be
> thread-safe, this will surely be missed.

But CAN it be fixed? I don't think it could, at least not without
wrapping ALL calls to functions that perform IO on file handles or
file descriptors...

Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>
>>>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD
>>
>> Sensible.
>>
>>>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather
>>>      than in git-compat-util.h
>>
>> I think it is a bad change.  When compat/ pread gets improved to be
>> thread-safe, this will surely be missed.
>
> But CAN it be fixed? I don't think it could, at least not without
> wrapping ALL calls to functions that perform IO on file handles or
> file descriptors...

Is that relevant?  It may be true that both Erik and Junio are not
being clever enough to come up with a solution offhand.  But is that
a good justification to go against a sound engineering practice?

There may be more than one platforms that want their own different
pread emulations, some safe with thread and some others not.  I
think you would prefer to see something like this:

	#if NO_PREAD
        #if on platform A
	# define NO_THREAD_SAFE_PREAD
        # define pread(fd, buf, count, ofs) git_pread_A((fd), (buf), (count), (ofs))
	#endif
        #if on platform B
        # define pread(fd, buf, count, ofs) git_pread_B((fd), (buf), (count), (ofs))
	#endif

and keep "make -DNO_PREAD" to mean "The platform does not have a
pread, please emulate" and nothing else. The implementation of the
emulation itself would be the one that knows of its thread safeness.

Having said that, I do not care too deeply either way, so the patch
is queued as-is.

Re: [PATCH v2 1/1] index-pack: Disable threading on cygwin

[Erik Faye-Lund]

On Wed, Jun 27, 2012 at 12:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
>>>
>>>>    - renamed FAKE_PREAD_NOT_THREAD_SAFE to NO_THREAD_SAFE_PREAD
>>>
>>> Sensible.
>>>
>>>>    - when NO_PREAD, set NO_THREAD_SAFE_PREAD in the Makefile, rather
>>>>      than in git-compat-util.h
>>>
>>> I think it is a bad change.  When compat/ pread gets improved to be
>>> thread-safe, this will surely be missed.
>>
>> But CAN it be fixed? I don't think it could, at least not without
>> wrapping ALL calls to functions that perform IO on file handles or
>> file descriptors...
>
> Is that relevant?  It may be true that both Erik and Junio are not
> being clever enough to come up with a solution offhand.  But is that
> a good justification to go against a sound engineering practice?
>

I didn't really mean to argue for or against this particular solution,
sorry if I was unclear. I'm more interested in actually fixing the
issue without disabling threading ;)

At least on Windows, the CRT actually does take a lock for the
file-handle for all standard IO opterations. Perhaps we can somehow
take it ourselves? I'm not entirely sure how to get hold of it, but it
would probably require quite a deep-dive into the CRT sources...