Menu
▾
▴
Re: [ jEdit-devel ] TokenMarker.java question
|
From: Shlomy R. <sre...@gm...> - 2009-10-03 16:18:30
|
It would (if possible), but that would require a larger change, which
has a potential of introducing another bug. The one I did just adds
the word "synchronized"... If there is a problem with that, I will
consider another change. I am not completely sure it can be made
reentrant, though.
Shlomy
On Sat, Oct 3, 2009 at 5:13 PM, Dale Anson <da...@gr...> wrote:
> woops, just saw your other email about TaskList.
>
> Wouldn't it be better to make TokenMarker reentrant rather than synchronized?
>
> Dale
>
>
> On Sat, Oct 3, 2009 at 9:08 AM, Dale Anson <da...@gr...> wrote:
>> What was the issue with TaskList? And which TaskList -- the one in
>> PluginManager or the one from Eric's daily builds?
>>
>> Dale
>>
>>
>> On Sat, Oct 3, 2009 at 7:39 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>> I have committed this small change (make TokenMarker.markTokens
>>> synchronized) and it seems to solve the issue with TaskList.
>>> I also haven't noticed any performance impact of this. If anyone
>>> discovers a performance problem caused by this, let me know and I'll
>>> fix it.
>>>
>>> Shlomy
>>>
>>> On Sat, Oct 3, 2009 at 11:40 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>> Hi,
>>>>
>>>> It turns out that TokenMarker.markTokens is not thread-safe. It needs
>>>> to be synchronized. It has a 'context' field, that is potentially
>>>> updated each time "markTokens" is called. If called simultaneously by
>>>> multiple threads, each may modify it, causing the other threads to use
>>>> an incorrect context.
>>>>
>>>> I just verified that the problem happens very frequently with the
>>>> TaskList plugin. TaskList has a dockable which shows the tasks in the
>>>> current buffer, and also highlights tasks in the text area. To find
>>>> the tasks in the buffer, it uses "markTokens" in the background. So,
>>>> when this plugin is in use, I experience all kinds of painting issues
>>>> and occasionally the NPE that Dale experienced.
>>>> Disabling the TaskList plugin gets rid of all these problems.
>>>>
>>>> However, it is not a TaskList problem, "markTokens" simply needs to be
>>>> synchronized. If this causes some performance problem in the core,
>>>> then we should have two separate paths to markTokens, one for the core
>>>> and one for plugins, and make the plugin's path ensure thread-safety,
>>>> e.g. by deferring the task to the AWT thread.
>>>>
>>>> Shlomy
>>>>
>>>> On Sat, Oct 3, 2009 at 3:39 AM, Dale Anson <da...@gr...> wrote:
>>>>> Actually, I verified that context.parent is occasionally null, which
>>>>> causes the NPE when its 'rules' field is referenced. Thinking about
>>>>> this some more, I bet is it's a threading issue. It could be that in
>>>>> the first check (in the code you sent) it is not null, but is reset to
>>>>> null before the method is actually invoked. That would mean my patch
>>>>> is not very good since it doesn't address thread-safety.
>>>>>
>>>>> I did double check, I've got source for jEdit at revision 16233. The
>>>>> only change I have is the one I made to this class. I verified that
>>>>> I'm using the standard java mode from svn also, and it's while editing
>>>>> java files that I know I've seen the problem. I'm not sure if I've
>>>>> seen this issue in other modes.
>>>>>
>>>>> Dale
>>>>>
>>>>>
>>>>> On Fri, Oct 2, 2009 at 2:46 AM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>> I took a more thorough look at the code. It is absolutely IMPOSSIBLE
>>>>>> that context.parent is null in the method that generates the NPE for
>>>>>> you, unless you've changed something in the jedit core. There is only
>>>>>> one place where the method generating the NPE is called:
>>>>>> //{{{ check for end of delegate
>>>>>> if (context.parent != null
>>>>>> && context.parent.inRule != null
>>>>>> && checkDelegateEnd(context.parent.inRule))
>>>>>> {
>>>>>> seenWhitespaceEnd = true;
>>>>>> continue main_loop;
>>>>>> } //}}}
>>>>>>
>>>>>> Note the check for "context.parent" not being null before calling this method.
>>>>>> An NPE can still be caused by this line, but it won't be for
>>>>>> "context.parent" being null, rather it would be for
>>>>>> "context.parent.rules" being null.
>>>>>>
>>>>>> Shlomy
>>>>>>
>>>>>> On Fri, Oct 2, 2009 at 8:15 AM, Dale Anson <da...@gr...> wrote:
>>>>>>> No, I can't reproduce it reliably. Somehow, I get into this state
>>>>>>> after a couple of hours of running jEdit. Usually, it happens when
>>>>>>> I'm editing somewhere in the middle of a file, I'll hit Enter at the
>>>>>>> end of a line to start the next line and then there is the trouble. I
>>>>>>> have verified that it is definitely the case that context.parent is
>>>>>>> sometimes null. I see in LineContext itself that there are a several
>>>>>>> checks for a null parent, but not elsewhere. I think I've only seen
>>>>>>> this with java files, but I'm not absolutely certain.
>>>>>>>
>>>>>>> Do you know of a better thing to do if context.parent is null?
>>>>>>>
>>>>>>> Dale
>>>>>>>
>>>>>>>
>>>>>>> On Thu, Oct 1, 2009 at 11:31 PM, Shlomy Reinstein <sre...@gm...> wrote:
>>>>>>>> Hi,
>>>>>>>> Can you reproduce this every time with some example?
>>>>>>>> If not , does this happen when you are editing (i.e. pressing enter
>>>>>>>> on) the first or second line, or also when editing subsequent lines
>>>>>>>> down the file?
>>>>>>>> The 'parent' is usually the LineContext of the previous line.
>>>>>>>> Shlomy
>>>>>>>>
>>>>>>>> On Fri, Oct 2, 2009 at 3:33 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>> I entered a patch to fix this, see tracker 2871624. I did some more
>>>>>>>>> investigation, and it turns out that context.parent can be null. The
>>>>>>>>> patch checks for this case, and if the parent is null, it just
>>>>>>>>> continues to use the same context. So far, this seems to be working
>>>>>>>>> fine.
>>>>>>>>>
>>>>>>>>> Dale
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Sep 30, 2009 at 1:07 PM, Alan Ezust <ala...@gm...> wrote:
>>>>>>>>>> I've seen an NPE there from time to time also, but I can't reproduce it.
>>>>>>>>>>
>>>>>>>>>> On Wed, Sep 30, 2009 at 11:50 AM, Dale Anson <da...@gr...> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Wed, Sep 30, 2009 at 9:58 AM, Matthieu Casanova
>>>>>>>>>>> <cho...@gm...> wrote:
>>>>>>>>>>> > But in that code, the fields are used as parameters, so filling the
>>>>>>>>>>> > keyword
>>>>>>>>>>> > field at line 294 is used to highlight tokens, removing that will lead
>>>>>>>>>>> > to
>>>>>>>>>>> > bugs.
>>>>>>>>>>> > For the NPE at line 294, do you know if it is because of context being
>>>>>>>>>>> > null
>>>>>>>>>>> > or context.rules ?
>>>>>>>>>>>
>>>>>>>>>>> I don't know. I didn't look into it far enough. I supposed, though,
>>>>>>>>>>> if I'm the only one seeing this issue, that means I've got something
>>>>>>>>>>> wrong somewhere.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ------------------------------------------------------------------------------
>>>>>>>>> Come build with us! The BlackBerry® Developer Conference in SF, CA
>>>>>>>>> is the only developer event you need to attend this year. Jumpstart your
>>>>>>>>> developing skills, take BlackBerry mobile applications to market and stay
>>>>>>>>> ahead of the curve. Join us from November 9-12, 2009. Register now!
>>>>>>>>> http://p.sf.net/sfu/devconf
>>>>>>>>> --
>>>>>>>>> -----------------------------------------------
>>>>>>>>> jEdit Developers' List
>>>>>>>>> jEd...@li...
>>>>>>>>> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry® Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9-12, 2009. Register now!
> http://p.sf.net/sfu/devconf
> --
> -----------------------------------------------
> jEdit Developers' List
> jEd...@li...
> https://lists.sourceforge.net/lists/listinfo/jedit-devel
>
|
