qcommit: cancel leave branch locked and prevent to close window

Bug #277082 reported by Alexander Belchenko
2
Affects Status Importance Assigned to Milestone
QBzr
Fix Released
High
Alexander Belchenko

Bug Description

If I'm trying to cancel already started commit operation then I can't close qcommit window. Click on Cancel again produce tarceback in the console:

C:\Temp\qci>bzr qci
Committing to: C:/Temp/qci/
added .bzrignore
added .rsyncexclude
added BRANCH.TODO
added COPYING.txt
added INSTALL
added Makefile
added NEWS
added README

<<< here I hit Cancel first time

<<< Then I hit Cancel second time and got traceback:

Unable to obtain lock file:///C:/Temp/qci/.bzr/branch/lock
held by <email address hidden> on host tolstihin [process #564]
locked 17 seconds ago
Will continue to try until 15:55:01

And then I need to kill window manually. And after this I need to run `bzr break-lock`.

Tags: qcommit
Revision history for this message
Alexander Belchenko (bialix) wrote :

Very annoying behavior.

Changed in qbzr:
importance: Undecided → High
Revision history for this message
Alexander Belchenko (bialix) wrote :

BTW, repository is unlocked, but branch and working tree is, here output of bzr info -v:

C:\Temp\qci>bzr info -v
Standalone tree (format: pack-0.92)
Location:
  branch root: .

Format:
       control: Meta directory format 1
  working tree: Working tree format 4
        branch: Branch format 6
    repository: Packs containing knits without subtree support

Lock status:
  working tree: locked
        branch: locked
    repository: unlocked

In the working tree:
         0 unchanged
         0 modified
       709 added
         0 removed
         0 renamed
         0 unknown
       128 ignored
        47 versioned subdirectories

Branch history:
         0 revisions
         0 committers

Repository:
         0 revisions
       104 KiB

Revision history for this message
Alexander Belchenko (bialix) wrote :

This was wtih bzr 1.5 and qbzr trunk. I'll try to check with newer bzr.

Revision history for this message
Alexander Belchenko (bialix) wrote :

Yep, the same behavior with bzr 1.7.1

Changed in qbzr:
status: New → Confirmed
Revision history for this message
Gary van der Merwe (garyvdm) wrote :

I've fixed this on *nix.

I'm looking into using http://msdn.microsoft.com/en-us/library/ms683155(VS.85).aspx to fix on windows.

Revision history for this message
Mark Hammond (mhammond) wrote :

GenerateConsoleCtrolEvent *might* be tricky to use in this case - IIUC, in practice it only works for processes that share the same console. In the case of tbzr, qbzr will not even have a console to share! I spent some time with the Zope project trying to use this function so a service could interrupt a child process of its own, but failed - we ended up implementing a win32 specific way to interrupt the child process.

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

I figured out why the window does not close. In the close_event, we try to save the commit message. Because the branch is locked, this hangs.

I took a look at what zope does. They use win32event to send a msg from one process to another. They have a separate thread which waits for a msg. What I'm sure about is how that thread causes the other thread to interrupt.

Revision history for this message
Gary van der Merwe (garyvdm) wrote :
Revision history for this message
Alexander Belchenko (bialix) wrote :

Is there any important reason why we cannot use support for killing process via Qt method? According to documentation:

QProcess.kill (self)

This method is also a Qt slot with the C++ signature void kill().

Kills the current process, causing it to exit immediately.

On Windows, kill() uses TerminateProcess, and on Unix and Mac OS X, the SIGKILL signal is sent to the process.

Revision history for this message
Alexander Belchenko (bialix) wrote :
Revision history for this message
Gary van der Merwe (garyvdm) wrote :

Alexander Belchenko wrote:
> Is there any important reason why we cannot use support for killing process via Qt method? ... On Windows, kill() uses TerminateProcess...

When you press ctr-c, a SIGINT signal is sent to your process. This causes python to raise a KeyboardInterupt exception in the current thread. Hence, if you have a try...finally block, and the execution was in the try block when the KeyboardInterupt exception was raised, the code in the finally block will run. This does not happen if you call TerminateProcess/SIGKILL. The most visible effect of this is that locks aren't unlocked.

Alexander Belchenko wrote:
> http://www.microsoft.com/msj/0698/win320698.aspx

This article mentions GenerateConsoleCtrlEvent. I have spent many hours trying to get this to work. One problem I have encounter is that you need to pass CREATE_NEW_PROCESS_ GROUP to CreateProcess. This call is warped in QProcess.start(). So if we were to use this, we would need to drop the use of QProcess, and rewrite a lot of code related to handling stdout/stderr.

Revision history for this message
Alexander Belchenko (bialix) wrote : Re: [Bug 277082] Re: qcommit: cancel leave branch locked and prevent to close window

Gary van der Merwe пишет:
> Alexander Belchenko wrote:
>> Is there any important reason why we cannot use support for killing process via Qt method? ... On Windows, kill() uses TerminateProcess...
>
> When you press ctr-c, a SIGINT signal is sent to your process. This
> causes python to raise a KeyboardInterupt exception in the current
> thread. Hence, if you have a try...finally block, and the execution was
> in the try block when the KeyboardInterupt exception was raised, the
> code in the finally block will run. This does not happen if you call
> TerminateProcess/SIGKILL. The most visible effect of this is that locks
> aren't unlocked.

Thanks for explanation, but current usage of os.kill anyway wrong on win32.

> Alexander Belchenko wrote:
>> http://www.microsoft.com/msj/0698/win320698.aspx
>
> This article mentions GenerateConsoleCtrlEvent. I have spent many hours
> trying to get this to work. One problem I have encounter is that you
> need to pass CREATE_NEW_PROCESS_ GROUP to CreateProcess. This call is
> warped in QProcess.start(). So if we were to use this, we would need to
> drop the use of QProcess, and rewrite a lot of code related to handling
> stdout/stderr.
>

Gary, you mentioned thread.interrupt_main function. Will it work for us?
I've played with win32event module, so I think we can send signal from main
GUI dialog to subprocess via win32 Events. And we can wait for the Event
in separate thread in qsubprocess to have ability using interrupt_main.
So I'm planning to test this idea, if you don't try it before me.

Revision history for this message
Gary van der Merwe (garyvdm) wrote :

I have tried that but I could not get the win32event signaling to work. I hope you have better luck!

Revision history for this message
Alexander Belchenko (bialix) wrote :

Gary van der Merwe пишет:
> I have tried that but I could not get the win32event signaling to work.
> I hope you have better luck!

I can send signal between 2 python applications with win32event in my recent
manual testing, so I'm hope I'll get it to work for QBzr too.

Revision history for this message
Alexander Belchenko (bialix) wrote :
Changed in qbzr:
assignee: nobody → bialix
milestone: none → 0.9.7
status: Confirmed → In Progress
Revision history for this message
Alexander Belchenko (bialix) wrote :
Revision history for this message
Alexander Belchenko (bialix) wrote :

works fine even with tbzr.

Changed in qbzr:
status: In Progress → Fix Committed
Changed in qbzr:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.