Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5252 closed Bug (fixed)

File-renaming causes "QObject::startTimer: timers cannot be started from another thread" error

Reported by: rb07 Owned by: jordan
Priority: Normal Milestone:
Component: Qt Client Version:
Severity: Normal Keywords:
Cc:

Description

Since r13810, and testing with r13863, file renaming only works partially: the file gets renamed, but the application stops refreshing the UI (for all torrents, and the opened details panel so you never see the change), and becomes unusable until you restart it (then it shows the changed file name).

My guess is that the call-back is the one starting the timer in a different thread.

session.cc:

Session :: localSessionCallback( tr_session * session, struct evbuffer * json, void * vself )
{
  Q_UNUSED (session);

  Session * self = static_cast<Session*>(vself);

  self->myIdleJSON.append (QString ((const char*) evbuffer_pullup (json, -1)));

  if (!self->myResponseTimer.isActive())
    self->myResponseTimer.start(50);
}

The error message in the summary is only shown in the console.

Change History (26)

comment:1 Changed 8 years ago by jordan

Interesting, I don't get that error message in the console whether I'm running a local or remote session. I wonder if it's platform-dependent.

Looking at http://cdumez.blogspot.com/2011/03/worker-thread-in-qt-using-signals-slots.html and http://stackoverflow.com/questions/6732583/qobjectstarttimer-timers-cannot-be-started-from-another-thread-without-time, it looks like a safer way to delegate work from the libtransmission thread to the Qt thread would be with signals/slots.

comment:2 Changed 8 years ago by jordan

  • Milestone None Set deleted
  • Status changed from new to assigned
  • Version 2.76+ deleted

comment:3 follow-up: Changed 8 years ago by jordan

rb07, in r13864 I've replaced the list-and-timer mechanism with signal-and-slot to push the response from the libtransmission thread to the Qt thread.

I can confirm that this new approach works for me.

However, since I didn't get the console message on the previous approach, I'd like you to give this code a try before we mark this ticket as 'fixed'.

comment:4 in reply to: ↑ 3 Changed 8 years ago by rb07

Replying to jordan:

However, since I didn't get the console message on the previous approach, I'd like you to give this code a try before we mark this ticket as 'fixed'.

Yes, it does fix the problem.

Thanks!

Side questions:

  1. I had to disable the keyboard shortcuts in file-tree.cc :
        // handle using the keyboard to toggle the
        // wanted/unwanted state or the file priority
    /*** DISABLED: its incompatible with renaming files.
        else if( event->type() == QEvent::KeyPress )
        {
            switch( dynamic_cast<QKeyEvent*>(event)->key() )
            {
                case Qt::Key_Space:
                    foreach( QModelIndex i, selectionModel()->selectedRows(COL_WANTED) )
                        clicked( i );
                    return false;
    
                case Qt::Key_Enter:
                case Qt::Key_Return:
                    foreach( QModelIndex i, selectionModel()->selectedRows(COL_PRIORITY) )
                        clicked( i );
                    return false;
            }
        }
    ***/
    

which obviously interfere with editing the file name. Do you intend to keep them?

  1. The file editing is also enabled in the (Open Torrent) options panel, where it doesn't work. Should editing be disabled there? or it should be working.
  1. Probably specific to my Windows port, but renaming is (or was) also not working on a file being downloaded, I get "access denied". Is this the case on Linux? (then I probably need to fix the flags so that an open file can be renamed). Actually this one just changed to "file doesn't exist", the checking of .part may be off (r13865 and local session).
Last edited 8 years ago by rb07 (previous) (diff)

comment:5 Changed 8 years ago by rb07

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:6 Changed 8 years ago by jordan

rb07, I think I've got items 1 and 2 fixed in r13893... please reopen this ticket if you see any remaining issues.

I'm not seeing 3 on my machine. :/

comment:7 Changed 8 years ago by rb07

  • Resolution fixed deleted
  • Status changed from closed to reopened

New issue w/r13894: Can't edit file names while downloading, the cursor keeps jumping to the end (every 3 or 4 sec) after replacing the string with the original. This (maybe) wasn't the case with the code from 2 days ago.

Another: Opening the torrent properties I get 2 identical lines in the console saying "I don't have this index".

Another: Something weird is going on while changing speed limits with the status bar drop down. The first click over a value doesn't do anything, or to be more precise, looks like it makes everything flicker, and doesn't change, or even select the speed. The second time it works, but if you close the drop down and open it again, the same thing happens.

Last edited 8 years ago by rb07 (previous) (diff)

comment:8 Changed 8 years ago by jordan

gaah, I think I know what's causing the jumping cursor. Thanks for the heads-up rb07.

That last paragraph sounds unrelated? You may want to file a separate ticket for that, as much as it pains me to say so because I'm trying to get the bug count down under 200... :)

comment:9 follow-up: Changed 8 years ago by jordan

rb07, the flicker & resetting value should be fixed now in r13930. Could you test & see if it works for you on Windows as well?

comment:10 in reply to: ↑ 9 Changed 8 years ago by rb07

Replying to jordan:

rb07, the flicker & resetting value should be fixed now in r13930. Could you test & see if it works for you on Windows as well?

Yes, the editing works fine now.

But changing the name while the torrent is downloading doesn't. I'm debugging to see what the actual problem is.

comment:11 Changed 8 years ago by rb07

Yep, its a Windows problem, I'm getting a:

ERROR_SHARING_VIOLATION

    32 (0x20)

    The process cannot access the file because it is being used by another process.

Its just another thread dumb Windows. Oh well, time to test some Windows flags: FILE_SHARE_DELETE sounds about right...

Last edited 8 years ago by rb07 (previous) (diff)

comment:12 follow-up: Changed 8 years ago by jordan

rb07, any news on this?

comment:13 in reply to: ↑ 12 ; follow-up: Changed 8 years ago by rb07

Replying to jordan:

rb07, any news on this?

Yes, I just made it work on Windows.

Since my code has a lot of modifications (remember ticket #4160?), I don't think its worth adding a patch to this ticket. If anybody likes to see what the changes are like:

int tr_open (const char *pathUTF8, int oflag, ...)
{
    va_list a;
    int mode = 0;
    va_start(a, oflag);
    mode = va_arg(a, int);
    va_end(a);
#ifdef WIN32
    if (pathUTF8) {
        int n = strlen(pathUTF8) + 1;
        wchar_t pathUTF16[n];
        if (MultiByteToWideChar(CP_UTF8, 0, pathUTF8, -1, pathUTF16, n))
            /* binary mode is mandatory, otherwise UTF16 data is assumed */
#if 0
            return _wsopen(chkFilename(pathUTF16), oflag | _O_BINARY, _SH_DENYNO, mode);
#endif
        {   HANDLE h;
            h = CreateFileW(chkFilename(pathUTF16),
                            GENERIC_READ|GENERIC_WRITE,
                            FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
                            NULL,
                            OPEN_ALWAYS,
                            FILE_FLAG_RANDOM_ACCESS,
                            NULL);
            if (h == INVALID_HANDLE_VALUE)
            {   errno = GetLastError();
                return -1;
            }
            return _open_osfhandle(h, oflag | _O_BINARY);
        }
    }
    errno = ENOENT;
    return -1;
#else
    return open(pathUTF8, oflag, mode);
#endif
}

FILE *tr_fopen (const char *restrict filenameUTF8, const char *restrict mode)
{   
#ifdef WIN32
    if (filenameUTF8) {
        int n = strlen(filenameUTF8) + 1;
        int m = strlen(mode) + 2;
        wchar_t filenameUTF16[n];
        wchar_t modeUTF16[m];
        MultiByteToWideChar(CP_UTF8, 0, filenameUTF8, -1, filenameUTF16, n);
        MultiByteToWideChar(CP_UTF8, 0, mode, -1, modeUTF16, m);
        /* binary mode is mandatory, otherwise UTF16 data is assumed */
        if (!strchr(mode, 'b'))
            wcscat(modeUTF16, L"b");
#if 0
        return _wfsopen(chkFilename(filenameUTF16), modeUTF16, _SH_DENYNO);
#endif
        {   HANDLE h;
            h = CreateFileW(chkFilename(filenameUTF16),
                            GENERIC_READ|GENERIC_WRITE,
                            FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
                            NULL,
                            OPEN_ALWAYS,
                            FILE_FLAG_RANDOM_ACCESS,
                            NULL);
            if (h == INVALID_HANDLE_VALUE)
            {   errno = GetLastError();
                return NULL;
            }
            return _wfdopen(_open_osfhandle(h, _O_BINARY), modeUTF16);
        }
    }
    return NULL;
#else
    return fopen(filenameUTF8, mode);
#endif
}

I'm not sure if I'm adding more problems (since the "mode" in the open() function is not even used.)

comment:14 follow-up: Changed 8 years ago by jordan

Actually I had forgotten #4160. *shame*

I'd like to shrink the diffs between our two branches. Could you attach a refresh patched to #4160?

comment:15 in reply to: ↑ 14 Changed 8 years ago by rb07

Replying to jordan:

Could you attach a refresh patched to #4160?

I don't think its worth the effort, my changes are not only about adding full support for foreign languages, I have added some 'features' (some proposed by other users in this tracker, some mine, some experimental, some unfinished), some bug fixes, and changes for using Qt5 (still unusable as of 5.0.1, it got worse w.r.t. plugins, and I use QtCurve style as a plugin).

Separating the different changes is a lot of work I don't want to do. So there is no clean patch for #4160.

comment:16 Changed 8 years ago by jordan

fair enough.

Is there a public repo where I can see the changes that you've made? The latest changeset I could find was a patch against 2.72.

comment:17 Changed 8 years ago by rb07

No, there is no public repository, I only use one which is my working copy of your Subversion repository. I don't keep track of my changes the way you do (yes I know, I shouldn't do that, its messy and bad practice).

I often forget to make a patch against released versions, that's why the last one I put on SF is against 2.72.

After the recent exchange in the forum ("your patches don't work", and "give me your code"), I'm rethinking the whole idea of making my code available (to people like that, or the other guy that gave you a patch, changing my code and breaking the way it worked on Windows probably on purpose because he didn't like my answers in the forum).

comment:18 Changed 8 years ago by jordan

Despite the bad attitude of some forum users, there are several really strong reasons to make the code available:

  • I'd like to merge parts of it into transmissionbt.com's repo, particularly portability patches. Ideally, the upstream code should be portable enough that downstream flavors can just add features without having to also fix issues like you're currently doing with remove(), rename(), et al.
  • It's required by section 3a of the license.
  • Transmission's openness is one of its biggest benefits to its users. If trqtw has unpublished patches, it opens itself to the same criticisms as other closed-source Windows clients. :(
Last edited 8 years ago by jordan (previous) (diff)

comment:20 Changed 8 years ago by jordan

rb07, any chance you could attach a transmission-2.76-Qt-build.diff to https://trac.transmissionbt.com/wiki/BuildingTransmissionQtWindows ...?

comment:21 Changed 8 years ago by rb07

No, I don't have it (as I said: I forgot to make the diff), and my 2.76 is not your 2.76 (i.e. I don't use the branch, I released based on the revision number -- which I didn't know Subversion duplicates, if I understand correctly, the rev. in head has the same number as one in the branch).

Anyway, if all you want is to see my current repository, then I can upload a diff to SF.

comment:22 Changed 8 years ago by jordan

That would be great. :)

comment:24 Changed 8 years ago by jordan

Thanks!

Moving past the discussion of code patches/repos... is there anything left to be done on this ticket?

comment:25 Changed 8 years ago by rb07

  • Resolution set to fixed
  • Status changed from reopened to closed

No.

comment:26 in reply to: ↑ 13 Changed 8 years ago by rb07

Replying to rb07:

Replying to jordan:

rb07, any news on this?

Yes, I just made it work on Windows.

For the record: No, I didn't.

Currently I can rename files at any time. But directories can only be renamed before they are created, and after download finishes.

The root of this (minor) problem is that mkdir in Windows doesn't have a way, or alternative function, to set share attributes.

I'm not sure if I'm adding more problems (since the "mode" in the open() function is not even used.)

I added mode handling.

Note: See TracTickets for help on using tickets.