#446 Convert WorkRequest to be more like a Task


Attached patch is the first step to convert the WorkRequest class and it's users to the Task class API.
It converts the WorkRequest to be more like a Task.
The background "task" queue is replaced by an ThreadPool. The Threads are created in the WorkThreadFactory class. The factory class created WorkThreads. The created WorkThreads keep track of the RequestCounter in the WorkThreadPool class and the AWT queue in WorkThreadPool serializes on behalf of this RequestCounter.


  • Matthieu Casanova

    Hi, I started reading your patch, It's a long one so I cannot read it entierely now.
    But as far as I understand, it does the same behavior we had in the old threadpool with the new threadpool.
    The benefit is that those tasks will be monitored at the same place.
    I think it is not bad.
    It will still be necessary to really remove the WorkRequests when someone have time to investigate on this, but the patch you submitted still seems a good thing until removing those WorkRequest is done.

  • Thomas Meyer

    Thomas Meyer - 2012-08-15


    there is a bug introduced by my step1 patch because of these two changes.
    These changes break the HyperSearch as it depends on the original behaviour.
    I would like to fix the HyperSearchRequest to be more strict in the waiting
    for the finishing of the IO.
    See the fix below.

    Should I drop these two changes?

    I want to keep the fix for the HyperSearchRequest.

    First the two problematic changes:

    diff -r 20cfa52d01fd org/gjt/sp/jedit/Buffer.java
    --- a/org/gjt/sp/jedit/Buffer.java Thu Aug 09 18:44:48 2012 +0200
    +++ b/org/gjt/sp/jedit/Buffer.java Mon Aug 13 17:37:52 2012 +0200
    @@ -299,10 +299,8 @@
    }; //}}}

    - if(getFlag(TEMPORARY))
    - runnable.run();
    - else
    - VFSManager.runInAWTThread(runnable);
    + // run in AWT thread
    + VFSManager.runInAWTThread(runnable);

    return true;
    } //}}}
    diff -r 20cfa52d01fd org/gjt/sp/jedit/io/VFS.java
    --- a/org/gjt/sp/jedit/io/VFS.java Thu Aug 09 18:44:48 2012 +0200
    +++ b/org/gjt/sp/jedit/io/VFS.java Mon Aug 13 17:37:52 2012 +0200
    @@ -490,14 +490,7 @@

    BufferIORequest request = new BufferLoadRequest(view, buffer, session, this, path);
    - if(buffer.isTemporary())
    - // this makes HyperSearch much faster
    - request.run();
    - else
    - // BufferLoadRequest can cause UI interations (for example FTP connection dialog),
    - // so it should be runned in Dispatch thread
    - //ThreadUtilities.runInDispatchThread(request);
    - VFSManager.runInWorkThread(request);
    + VFSManager.runInWorkThread(request);

    return true;
    } //}}}

    Fix HyperSearchRequest. A call to jEdit.openTemporary must wait for the IO
    to finish before using the buffer.
    Other users do this correctly, for example:
    I'm not sure if PerspectiveHandler.endElement() is correct.

    Anyway the correct thing to do is to wait for the IO to finish.

    diff -r 2cee01b37540 org/gjt/sp/jedit/search/HyperSearchRequest.java
    --- a/org/gjt/sp/jedit/search/HyperSearchRequest.java Wed Aug 15 20:07:03 2012 +0200
    +++ b/org/gjt/sp/jedit/search/HyperSearchRequest.java Wed Aug 15 20:12:11 2012 +0200
    @@ -26,6 +26,7 @@
    import javax.swing.tree.*;
    import javax.swing.*;

    +import org.gjt.sp.jedit.io.VFSManager;
    import org.gjt.sp.jedit.textarea.Selection;
    import org.gjt.sp.jedit.textarea.JEditTextArea;
    import org.gjt.sp.jedit.Buffer;
    @@ -138,7 +139,13 @@

    Buffer buffer = jEdit.openTemporary(null,null,file,false);
    if(buffer != null)
    + {
    + // Wait for the buffer to load
    + if(!buffer.isLoaded())
    + VFSManager.waitForRequests();
    resultCount += doHyperSearch(buffer, 0, buffer.getLength());
    + }
    Log.log(Log.MESSAGE, this, resultCount +" OCCURENCES");

  • Alan Ezust

    Alan Ezust - 2012-08-15

    I think the correct procedure is to first submit a patch for the hypersearchrequest and related fixes as a separate patch. Then add a comment to this ticket saying that the other patch should be applied first.

  • Thomas Meyer

    Thomas Meyer - 2012-08-16

    new version of the patch attached. this one fixes the hypersearch.

  • Alan Ezust

    Alan Ezust - 2012-08-17

    if v4 completely replaces v3, you can remove v3 from the list of attachments.

  • Alan Ezust

    Alan Ezust - 2012-08-17

    also, why on line 206 of the v4 patch, is there a @since jEdit 2.6pre1?

  • Thomas Meyer

    Thomas Meyer - 2012-08-21

    The patch relocates a few methods from WorkThread class to WorkRequest class. This is the reason for the "@since jEdit 2.6pre1" comment

  • Alan Ezust

    Alan Ezust - 2012-08-22
    • assigned_to: nobody --> ezust
  • Thomas Meyer

    Thomas Meyer - 2012-08-25

    Updated patch attach. This one fixes WorkThreadPool.waitForRequests().

  • Jarek Czekalski

    Jarek Czekalski - 2012-08-25

    In line 187 of WorkThreadPool there is a lock, that was absent before. I guess it adds a new possibility of deadlock, in the scenario:

    1. a background request R1 is added, that creates another request; the request does not run at the moment; or does run but does not yet reach the critical point of creation another request
    2. edt thread calls waitForRequests
    3. R1 is executed, but cannot add a new request because the lock is held

    A similar danger appears in line 415, where non-blocking call is replaced with a call that may block.

  • Alan Ezust

    Alan Ezust - 2012-08-25

    WorkThreadPool is gone after step 3 though

  • Thomas Meyer

    Thomas Meyer - 2012-08-26

    Hi Jarek, thanks for looking at this patch. I want this patch to be correct,
    although this class will be gone in step3.

    "1. a background request R1 is added, that creates another request; the
    request does not run at the moment; or does run but does not yet reach the
    critical point of creation another request"

    So The background requst R1 is going to call "addWorkRequest()", correct?
    Because the background task is already queued, the counter "requestCount" will
    be > 0

    "2. edt thread calls waitForRequests"

    As a background task is currently running ("requestCount > 0") the call to
    "waitForRequests()" will not leave this loop "while(requestCount.get() != 0)"
    waiting for the "waitForAllLock", which is notified by every background process
    which finishes. So the call to waitForRequests() will not reach line 187 until
    all background task finishes.
    Given that the background request R1 will queue another request R2 the
    counter "requestCount" will be increased and make the caller of
    "waitForRequests()" wait even longer.

    "3. R1 is executed, but cannot add a new request because the lock is held"

    No lock is held at this point because of above arguments.

    Do I have an error in my reasoning here?

    "A similar danger appears in line 415, where non-blocking call is replaced
    with a call that may block."

    Yes, I think this is an error. Fixed patch attached.

  • Jarek Czekalski

    Jarek Czekalski - 2012-08-26

    Thomas, your reasoning is correct. Thanks for putting it down. However calling unknown code (request code) while holding a lock must be exploitable. It required another hour of digging but finally I did it. See attached macro, that makes jedit stall. Trunk version passes this test.

    You guys say that this code is removed. Ok, but it is replaced by other code. This other code is based on something. Now is the base correct? Is the base modified?

    Reviewing all these patches exceeds my capabilities. I only wanted to point out the danger of restructuring the threading api. Maybe my comments on removed code will be of any use still.

  • Jarek Czekalski

    Jarek Czekalski - 2012-08-26

    a macro deadlocking v6

  • Thomas Meyer

    Thomas Meyer - 2012-08-27

    Hi Jarek,

    thanks for this excellent test case. Fixed patch attach. I rebased the other patches of this series and did check each patch for this problem. It should work now correctly in each step.

  • Thomas Meyer

    Thomas Meyer - 2012-08-30

    Some more fixes. I want to commit this version, so please review and test.

  • Thomas Meyer

    Thomas Meyer - 2012-09-01

    one more change, regarding Jarek's latest javadoc change.

  • Alan Ezust

    Alan Ezust - 2012-09-02
    • assigned_to: ezust --> thomasmey
  • Thomas Meyer

    Thomas Meyer - 2012-09-05
    • status: open --> closed

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

No, thanks