_Singleton leaves an open handle

[smartee]

It creates a mutex, however if the named mutex already exists, it gets a handle to it (that can be used with WaitForSingleObject) but it does nothing with this handle, it instead returns 0 and sets @error to ERROR_ALREADY_EXISTS. Great for a one time check at the beginning of a script, but it essentially kills its own post-run functionality by leaving the handle open.

Suggested fix:
After this line

If $lastError[0] = $ERROR_ALREADY_EXISTS Then

Drop in these lines to close the handle

DllCall("kernel32.dll", "bool", "CloseHandle", "handle", $handle[0])
If @error Then Return SetError(@error, @extended, 0)

So the updated function will look like this:

; #FUNCTION# ====================================================================================================================
; Name...........: _Singleton
; Description ...: Enforce a design paradigm where only one instance of the script may be running.
; Syntax.........: _Singleton($sOccurenceName[, $iFlag = 0])
; Parameters ....: $sOccurenceName - String to identify the occurrence of the script.  This string may not contain the \ character unless you are placing the object in a namespace (See Remarks).
;                 $iFlag          - Behavior options.
;                 |0 - Exit the script with the exit code -1 if another instance already exists.
;                 |1 - Return from the function without exiting the script.
;                 |2 - Allow the object to be accessed by anybody in the system. This is useful if specifying a "Global\" object in a multi-user environment.
; Return values .: Success    - The handle to the object used for synchronization (a mutex).
;                 Failure     - 0
; Author ........: Valik
; Modified.......:
; Remarks .......: You can place the object in a namespace by prefixing your object name with either "Global\" or "Local\".  "Global\" objects combined with the flag 2 are useful in multi-user environments.
; Related .......:
; Link ..........:
; Example .......: Yes
; ===============================================================================================================================
Func _Singleton($sOccurenceName, $iFlag = 0)
Local Const $ERROR_ALREADY_EXISTS = 183
Local Const $SECURITY_DESCRIPTOR_REVISION = 1
Local $tSecurityAttributes = 0
If BitAND($iFlag, 2) Then
  ; The size of SECURITY_DESCRIPTOR is 20 bytes.  We just
  ; need a block of memory the right size, we aren't going to
  ; access any members directly so it's not important what
  ; the members are, just that the total size is correct.
  Local $tSecurityDescriptor = DllStructCreate("byte;byte;word;ptr[4]")
  ; Initialize the security descriptor.
  Local $aRet = DllCall("advapi32.dll", "bool", "InitializeSecurityDescriptor", _
    "struct*", $tSecurityDescriptor, "dword", $SECURITY_DESCRIPTOR_REVISION)
  If @error Then Return SetError(@error, @extended, 0)
  If $aRet[0] Then
   ; Add the NULL DACL specifying access to everybody.
   $aRet = DllCall("advapi32.dll", "bool", "SetSecurityDescriptorDacl", _
     "struct*", $tSecurityDescriptor, "bool", 1, "ptr", 0, "bool", 0)
   If @error Then Return SetError(@error, @extended, 0)
   If $aRet[0] Then
    ; Create a SECURITY_ATTRIBUTES structure.
    $tSecurityAttributes = DllStructCreate($tagSECURITY_ATTRIBUTES)
    ; Assign the members.
    DllStructSetData($tSecurityAttributes, 1, DllStructGetSize($tSecurityAttributes))
    DllStructSetData($tSecurityAttributes, 2, DllStructGetPtr($tSecurityDescriptor))
    DllStructSetData($tSecurityAttributes, 3, 0)
   EndIf
  EndIf
EndIf
Local $handle = DllCall("kernel32.dll", "handle", "CreateMutexW", "struct*", $tSecurityAttributes, "bool", 1, "wstr", $sOccurenceName)
If @error Then Return SetError(@error, @extended, 0)
Local $lastError = DllCall("kernel32.dll", "dword", "GetLastError")
If @error Then Return SetError(@error, @extended, 0)
If $lastError[0] = $ERROR_ALREADY_EXISTS Then
  DllCall("kernel32.dll", "bool", "CloseHandle", "handle", $handle[0])
  If @error Then Return SetError(@error, @extended, 0)
  If BitAND($iFlag, 1) Then
   Return SetError($lastError[0], $lastError[0], 0)
  Else
   Exit -1
  EndIf
EndIf
Return $handle[0]
EndFunc   ;==>_Singleton

Thread showing issue:
​http://www.autoitscript.com/forum/topic/136745-solved-run-scripts-in-sequence/

[Valik]

Do you actually understand what the singleton pattern is? The whole point of the singleton pattern is to ensure that one and only one instance of the object (In this case the script) exists at a time. The function is already broken by returning anything at all. My original version exited but somebody who doesn't know what they are doing thought it would be a great idea to break the pattern for whatever asinine reason it was changed. Now you want to further break the function by further modifying already broken behavior? No, if I consent to any change to this function it will be to restore it back to it's true singleton pattern implementation.

Closing as no bug.

[anonymous]

The point raised (and solution provided) by smartee was (now implemented in misc.au3) was valid. I'm quite sure he understood the function of _Singleton(), and - more importantly - it would seem that he had run into the fact that _Singleton() sometimes just-doesn't-work as_documented.

As a matter of fact, _Singleton() was badly broken when "The whole point of the singleton pattern is to ensure that one and only one instance of the object (In this case the script) exists at a time." That may have been Valik's intent when he created _Singleton(), but that is certainly not how his code ever worked.

To begin with, _Singleton() did not (and still does not) create a "Global\*" mutex by default. So, as it stood (and still stands even in smartee's version), _Singleton() only worked when the session was also the same. "Pattern" broken, claim to "ensure that one and only one instance of the object" exists couldn't be upheld.

Then, Valik let CreateMutex() default the security attribute. So, _Singleton() only worked when the user was also the same. Again, "pattern" broken, claim to "ensure that one and only one instance of the object" exists couldn't be upheld.

Then there was the fact that _Singleton() gave the caller the option of returning from the call. Valik's original function Exit()ed, but that is just plain bad design to begin with. Not only does it not provide the caller with the option of notifying the user of the condition, or switching to the other window, or whatever, but it also cuts off any chance of passing back an unexpected error condition that the caller should have to deal with. For example, an invalid name as parameter (e.g. containing slashes).

Then, by not closing the handle upon ERROR_ALREADY_EXISTS, the original code was violating good programming practices. This led to the implicit (undocumented) assumption that the user would not retry. The documentation did not tell the user about this assumption. And, when the assumption turned out to be wrong, _Singleton() behaved in a manner that was also contradictory to "ensuring that one and only one instance of the object".

Incidentally, why was (and still is) _Singleton() creating the mutex with an initial lock? That is not appropriate if "the whole point" ... "is to ensure that one and only one instance of the object".

The claim that _Singleton() can "ensure" anything at all is pretty bold given that _Singleton() is dynamically interpreted code with quite a few obvious possible points of failure. The most basic of these was that there is nothing sane that it can do if any DllCall() fails.

Ultimately, _Singleton()'s promise to "Enforce a design paradigm where only one instance of the script may be running" is fundamentally impossible to keep as long as _Singleton() is a UDF.

smartee's solution, now implemented, is a good workaround for what is a fundamentally broken idea. It's probably a better idea to get replace _Singleton() with a native code builtin-function. But as long as that is not an option, then smartee's solution is gold.

My 2 cents on an old issue, but it needs to be pointed out that it was Valiks original _Singleton() that "was broken to begin with". Its smartee's source that is now in misc.au3, and he deserves credit for it.