threads / patch / 30909

v2index-pack: Disable threading on cygwin

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

## tl;dr

5 messages between Jun 26, 2012 and Jun 26, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Ramsay Jones· Jun 26, 2012, 18:19 UTC · lore
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(-)
Show changes to 2 files +10 −2

Makefile, builtin/index-pack.c

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
Junio C Hamano· Jun 26, 2012, 19:56 UTC · re: Ramsay Jones · lore

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

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.
Show 61 quoted lines
> 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
Erik Faye-Lund· Jun 26, 2012, 21:00 UTC · re: Junio C Hamano · lore

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

On Tue, Jun 26, 2012 at 9:56 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
> 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...

Junio C Hamano· Jun 26, 2012, 22:37 UTC · re: Erik Faye-Lund · lore

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

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 16 quoted lines
> 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.

Erik Faye-Lund· Jun 26, 2012, 22:57 UTC · re: Junio C Hamano · lore

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

On Wed, Jun 27, 2012 at 12:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> 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...

← back to recent threads