Bug 267531 - Large pastes can lock up Term/AllOfNetBeans
Large pastes can lock up Term/AllOfNetBeans
Status: STARTED
Product: cnd
Classification: Unclassified
Component: Terminalemulator
8.2
All All
: P1 (vote)
: Dev
Assigned To: ilia
issues@cnd
82patch-candidate
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-11 04:54 UTC by ivan
Modified: 2017-07-03 14:07 UTC (History)
1 user (show)

See Also:
Issue Type: DEFECT
:


Attachments
Patch for native part (5.16 KB, patch)
2017-02-22 07:05 UTC, ilia
Details | Diff
Patch for java part (4.06 KB, patch)
2017-02-22 07:30 UTC, ilia
Details | Diff
Thread Dump (49.16 KB, text/plain)
2017-07-03 14:07 UTC, ilia
Details

Note You need to log in before you can comment on or make changes to this bug.
Description ivan 2016-08-11 04:54:34 UTC
Start a terminal.
Edit a largish file in the NB editor.
Select all of the text in the editor.
Paste into the terminal.
NB locks up.
Comment 1 ivan 2016-08-11 04:58:09 UTC
There's a lot of "piping" involved and Term processes both pastes and their rendering on the EDT so it's quite possible that there's a deadlock because
pipes get full.

The remedy is prolly to have Term.pasteHelp() divvy up the pasted 
contents into chunks and use invokeLater() to submit them.
Comment 2 ilia 2017-02-22 07:05:03 UTC
Created attachment 163686 [details]
Patch for native part
Comment 3 ilia 2017-02-22 07:30:02 UTC
Created attachment 163687 [details]
Patch for java part
Comment 4 Quality Engineering 2017-03-03 03:54:46 UTC
Integrated into 'main-silver', will be available in build *201703030002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/ed91238ec9d9
User: Ilia Gromov <ilia@netbeans.org>
Log: Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
Less warning
(transplanted from 2bee8476b3f6908ff5bf7092c28295cd12b55f4d)
Comment 5 ilia 2017-03-13 09:22:19 UTC
As discussed with Ivan via e-mail:

-- Ilia

There are two deadlocks:
The minor one:
Is on the Java side. [See attachment, page #2]

    User pastes a large chunk of text into Term
    pasteHelp calls InputMonitor::sendChars(char[] buf, int offset, int n)
    We start a heavy writing operation from EDT here. EDT is blocked until the write is complete.
    loop.c receives a half of out text from STDIN (fd=0) and feed a process with it (lets assume we launched 'cat'). 'cat' spits out some output.
    loop.c directs this output to STDOUT (fd=1)
    loop in OutputMonitor::run receives text and tries to
    SwingUtilities.invokeAndWait(tramp).
    Deadlock. We are waiting EDT to complete a submitted read task but it will never be executed because step 2 is not completed yet and all pipes are full.

The major one:
Let's imagine we've fixed a Java side. But there is another problem, deadlock in loop.c [See attachment, page #1]:
Simplistically loop.c looks like:

    0: while(true) {
    1:      fds = 0, 3; // where 0 is STDIN(r) and 3 is master_fs(w,r)
    2:      poll fds
    3: 
    4:      if (<0> has data) {
    5:          read <0>
    6:          writen <3>
    7:      }
    8:     
    9:      if (<3> has data) {
    10:         read <3>
    11:         writen <1>
    12:     }
    12: }

Process behind <3> always answers with some delay. And this situation is appearing sometimes:
strace:

    restart_syscall(<... resuming interrupted restart_syscall ...>) = 1
    read(0, "/*\n * DO NOT ALTER OR REMOVE COP"..., 8192) = 8192
    write(3, "/*\n * DO NOT ALTER OR REMOVE COP"..., 8192) = 8192
    poll([{fd=0, events=POLLIN}, {fd=3, events=POLLIN}], 2, -1) = 2 ([{fd=0, revents=POLLIN}, {fd=3, revents=POLLIN}])
    read(0, "f lateral thinking.\n * </dl>\n *\n"..., 8192) = 8192
    write(3, "f lateral thinking.\n * </dl>\n *\n"..., 8192) =


We write to <3> several times and never read because cat is gives a response with a delay.
After couple of writes we block at line 6 because pipes behind master_fd (fd=3) are full.
What's the cure for this situation? Right, we need to read from full pipes, line 10. But... We'll never reach this line because we are busy writing to full pipes.

The solution to a native deadlock is separating STDIN->ptmx and ptmx->STDOUT loops to two different threads.
Maybe the Java's deadlock will disappear after the native fix. Maybe not (I'm sure it won't). But decisions for Java part will be easier
Comment 6 ilia 2017-03-13 09:24:11 UTC
-- Ivan

I tried reproducing this problem with my test terminal
    That's the .../lib.terminalemulator/examples/TermApp project
but couldn't. TermApp uses another project, RichExecution, which uses JNA to
access pty's directly from the VM. As a result you don't have a second
"shuttle" like CND's 'pty' copying things back and forth and things are simpler.

One thing I'm sure of it's not an EDT deadlock issue because xterm, konsole etc
are single-threaded, i.e. everything runs on their equivalent of EDT, and they
don't have problems. So moving things off the EDT is _not_ the solution.

I took a quick look at xterm code and it seems to write a bit at a time
and buffer the pasted characters but I haven't found where it loop's
over the buffer.
If you can get your hands on xterm source code look for MAX_PTY_WRITE
and v_write() in "charproc.c". 

-- Ivan

More on xterm.
Everything happens in charproc.c. 

in_put() is the input processing (event dispatch) function.
Make sure you look at the #ifndef VMS side.

At the beginning it reads input and interprets it. Near the end
it checks if it can write out remaining output and calls v_write(, 0, 0)
which has the effect of flushing the output buffers. These output
buffers are declared right above v_write() and in effect form a queue.

couple of important points.
- v_write depends on write() failure to learn that the buffer it's writing too
  is full, so I"m not sure why it bothers with MAX_PTY_WRITE perhaps because of
  the flakey error reporting?
- Before calling v_write() in_put() will call select, poll() really, on the _output_
  file descriptor. He purpose of this (you'll have to read the man page for poll
  carefully. look for POLLOUT) is to make sure that the output pipe has been drained
  and that the write won't block.

We don't have access to poll in java so I"ll take back what I said re offloading
the EDT. One solution might be to create an RP (not a netbeans RP because lib.term code
is supposed to not depend on NB API's) that flushes an output buffer (analogous to xterms
v_buffer) in a loop. A slightly easier solution might be to change the invokeAndWait()
at step (6) into invokeLater() and give it a _copy_ of the input. But since terminal usually
has to process more output this might add some overhead (handling a flood of output) 

I think buffering a'la xterm and using poll() for _output_
needs to be considered. Or maybe make it be multi-threaded.
Comment 7 ilia 2017-03-13 09:25:06 UTC
-- Ilia

> I think buffering a'la xterm and using poll() for _output_
> needs to be considered.

https://netbeans.org/bugzilla/attachment.cgi?id=163686&action=diff
I made a buffer for STDIN->master_fd writes (writings?). We assume master_fd->STDOUT writes are not in trouble because java reads incoming bytes almost without blocking. Also there is now writen_no_block function which helps to survive when the next write is about to block. In that case it returns control to the main loop and doesn't read any new data from STDIN until all bytes from buffer will be written to master_fd without block. (see util.c)

Also I reduced read/write max. block size from BUF_SIZ=8096 to FRAG_SIZ=1000 (xterm operates smaller chunks of data too.)

As for the Java part:
https://netbeans.org/bugzilla/attachment.cgi?id=163687&action=diff
I moved all writes from EDT to another thread. Also I'll take a look at Java 7 Pipe class, maybe that will help to solve Java deadlock without moving stuff away from EDT.
https://docs.oracle.com/javase/7/docs/api/java/nio/channels/Pipe.html
Because sumbitting tasks to another thread is a very expensive pleasure.

With all that fixes any amount of data can be pasted with no issues.
Comment 8 ilia 2017-03-13 09:27:26 UTC
-- Ivan

I don't think changing the buffer size makes much of a difference if you do
all the other async work. I usually change one thing at a time and evaluate
it's effect.

    The rationale for smaller buffers is that when you do a write, i.e. make a potentially blocking
    system call, the kernel gets an opportunity to schedule some other process, perhaps
    the consuming process, and things can work more smootly ...maybe.
    But it's no guarantee!

Anyway, I think you solution is too simple to always work. Consider this scenario:

in main loop you read into to_master.buf
then you try to writen_no_block() but the output fd isn't ready so nothing
gets sent out
Back in the main loop you poll but output fd _still_ isn't ready but
more input has come in so you read again and _overwrite_ what was left in to_master.buf!

I also think you need to do this for both directions "Java seems to read fast
enough" won't cut it.

A note on C programming practice.
Always check the return value of system calls! 

>
> As for the Java part:
> https://netbeans.org/bugzilla/attachment.cgi?id=163687&action=diff
> I moved all writes from EDT to another thread. Also I'll take a look at Java 7 Pipe class, maybe that will help to solve Java deadlock without moving stuff away from EDT.
> https://docs.oracle.com/javase/7/docs/api/java/nio/channels/Pipe.html
> Because sumbitting tasks to another thread is a very expensive pleasure.

Yes ExecutorService.submit() will create a new thread everytime it seems.

You also need to pay attention to shutdown ... Terminal's come and go so how many
ExecutorServices will you end up with after a long IDE session? 

But there's a more significant problem I see. sendChars()'s parameter 'c' is declared
'final' but are the _contents_ of that buffer final? Perhaps 'c' is "pointing" to
some other buffer that will get overwritten before the submitted job has run and written
the contents out? It's a variation of the same problem as in the C code.
Comment 9 ilia 2017-03-13 09:29:20 UTC
>  Consider this scenario:
>
> in main loop you read into to_master.buf
> then you try to writen_no_block() but the output fd isn't ready so nothing
> gets sent out
> Back in the main loop you poll but output fd _still_ isn't ready but
> more input has come in so you read again and _overwrite_ what was left in to_master.buf! 
We can't overwrite data to to_master.buf.
Ok, struct buffer to_master has a misleading name. Let's suppose it's name is struct storage to_master
to_master doesn't act like buffer in the conventional sense.

to_master only stores data which was not written to the master_fd last time (because it would block).
The most important thing: loop _won't_ read any bytes from STDIN until to_master.buf is drained (until to_master.offset == to_master.length).
When to_master.buf is not drained loop tries to do only two things:
* drain to_master.buf (write data from to_master.buf storage to master_fd).
* read data from master_fd and write it to STDOUT, thus freeng blocked pipes.
Loop doesn't even try to read any data from STDIN until to_master.buf is "empty"

> A simpler truly open-ended
> "buffer" scheme would be a queue of buffer objects with read() adding to the tail of
> the queue and write advancing through the buffer at the head of the queue
> until it's all written and then moving on to the next item in the queue. 
I agree that it's a naive buffer that fits our needs. But since we moved Java part
from EDT I think that buffering things on the Java side is a better solution.
As for C - we store only BUFSIZ=8192 chars at max.

> Yes ExecutorService.submit() will create a new thread everytime it seems.
Our executor is single-threaded. So no new threads are created - when submitting a new task executor queues it until thread is ready for a new task).
Acts like some kind of a buffer, actually. But anyway, even submitting a Runnable can be expensive. That's why I was thinking about Pipe class.

> But there's a more significant problem I see. sendChars()'s parameter 'c' is declared
> 'final' but are the _contents_ of that buffer final?
You are right, contents are mutable.
But every client of StreamTerm seems to make a defensive copy of char array it sends and
doesn't use this copy after sendChars is called

Defensive copying in StreamTerm::sendChars(...) can be done, but isn't it better to add a JavaDoc
saying that this call is async and only copies of char[] can be passed there?
But it isn't a big deal to add System.arraycopy call in StreamTerm::sendChars just to be sure that
new clients won't be fooled expecting their buffer won't be accessed asynchronously after they call sendChars.
GC will delete the original array anyway.
Comment 10 ilia 2017-03-13 09:31:04 UTC
-- Ivan

Then how is this complicated algorithm any different from what was there before?

Before your fix:
    loop()
        read
        write
no reads until blocked write finishes

After your fix:
_also_ no reads until write finishes:
    Loop doesn't even try to read any data from STDIN until to_master.buf is "empty"

In both cases things will get jammed except that in the second case you'll
be spinning your wheels. 

> Defensive copying in StreamTerm::sendChars(...) can be done, but isn't it better to add a JavaDoc
> saying that this call is async and only copies of char[] can be passed there? 

No it's not better to enhance javadoc. Who ever reads the fine print?

The toCharArray()s aren't defensive copyings. They are a conversions from String to char[].
The fact that the conversion produces a _copy_ is "luck" in this context.
The API says "I receive char[]'s" and it is supposed to work correctly if you satisy the API
and pass it a char[])

The reason the API receives char[] and char's (as opposed to Strings) is that it is low-level
and eventually will feed into Streams so it matches Streams API. Also the whole LineDiscipline
stuff uses char arrays so if you add Strings all of that becomes more complicated.
Comment 11 ilia 2017-03-13 09:32:00 UTC
-- Ilia

> After your fix:
> _also_ no reads until write finishes:
>     Loop doesn't even try to read any data from STDIN until to_master.buf is "empty" 
But it is able to read data from master_fd now. That's the main difference.
Reading master_fd frees blocked pipes. Including the one which doesn't allow to write to master_fd without  block. 

Possible alternative to this approach:
I see StreamTerm.Pipe class (which has no usages).
Won't it be better if we:
* write to PipedWriter from EDT
* read from PipedReader with a separate thread in a loop, like the OutputMonitor does?
At least we won't have a buffer (queue) of Runnables as in the approach with a thread pool,
but a buffer of chars (Pipe class with it's piped readers/writers).
Comment 12 ilia 2017-03-13 09:34:08 UTC
-- Ivan

> But it is able to read data from master_fd now. That's the main difference.
> Reading master_fd frees blocked pipes. Including the one which doesn't allow to write to master_fd without  block.

Ok.
So from STDIN to master_fd uses to_master and and won't read until to_master has all
been written out. From master_fd to STDOUT uses 'buf' and it assumes that the IDE on
the other side will never block. 

> Possible alternative to this approach:
> I see StreamTerm.Pipe class (which has no usages).

StreamPipe.Pipe does have usage just not in NB.
It is used for letting a terminal interact with a "process"
inside the VM itself. For example you may have a read-eval-print loop
running in the same VM as the terminal but in a different thread
and would like to provide some IO for the REP w/o going outside of the VM.
I have tons of programs that do this. 


> At least we won't have a buffer (queue) of Runnables as in the approach with a thread pool,
> but a buffer of chars (Pipe class with it's piped readers/writers).

I'm not too worried about queues of Runnables. Runnables are just lambdas not whole
threads and are supposed to be cheap. The EDT itself dispatches gazillions of them
everytime you move your mouse quickly or press on an auto-repeating
key plus everything else that happens in the GUI.
Comment 13 ilia 2017-03-13 09:35:02 UTC
Will integrate changes to trunk today.
Still need to modify C code for #ifdef __APPLE__
Comment 14 ilia 2017-03-13 09:45:13 UTC
Recompiled binaries for Linux_x86_64 and Solaris_x86_64
Comment 15 Quality Engineering 2017-03-15 02:50:24 UTC
Integrated into 'main-silver', will be available in build *201703150002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/dfb7bb32b9bb
User: Ilia Gromov <ilia@netbeans.org>
Log: Fixing #Bug 267531 - Large pastes can lock up Term/AllOfNetBeans
Copy char array (good API practice).
(transplanted from 446fdcbf622987200fcc7beb1dd28a1192539551)
Comment 16 ilia 2017-03-15 20:36:39 UTC
Need to check if thread pool is released when Terminal is destroyed.
Comment 17 ilia 2017-03-15 20:36:48 UTC
Need to check if thread pool is released when Terminal is destroyed.
Comment 18 Vladimir Voskresensky 2017-03-17 17:58:39 UTC
My experience:
big pastes works great!

But the behaviour of terminal after big pastes has the following issues:
- paste big run command
- press enter to run
- press UP to show run command again
-- it occupies multiple lines and cursor is on the last line which is GOOD
- press left toward left border or teminal
-- when cursor reaches the zero column it jumps to the last column, BUT of the SAME, not PREVIOUS line
Comment 19 Quality Engineering 2017-04-11 01:53:06 UTC
Integrated into 'main-silver', will be available in build *201704110002* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)

Changeset: http://hg.netbeans.org/main-silver/rev/a10777ed9097
User: Ilia Gromov <ilia@netbeans.org>
Log: Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans. Recompiled binaries for Windows 32bit, 64bit
(transplanted from df02bd4e7d6ff55c9460280e45fa7e72309698c1)
Comment 20 ilia 2017-04-12 12:46:17 UTC
Some binaries are still not recompiled but changesets are transplanted anyway:

https://netbeans.org/bugzilla/show_bug.cgi?id=267531 Changesets:
    http://hg.netbeans.org/releases/rev/2bee8476b3f6 # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/6ab106cff411 # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/38d038db38f5 # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/8373831bdbf0 # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/51595926ae90 # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/e9f849953db1 # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/446fdcbf6229 # Fixing #Bug 267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/870d38b05a8f # Fixing #Bug 267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/5b719a903825 # Fixing #Bug 267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/2e7b24718f41 # Fixing #Bug 267531 - Large pastes can lock up Term/AllOfNetBeans
    http://hg.netbeans.org/releases/rev/df02bd4e7d6f # Fixing #267531 - Large pastes can lock up Term/AllOfNetBeans. Recompiled binaries for Windows 32bit, 64bit
Comment 21 ilia 2017-04-25 16:31:15 UTC
Need to recompile binaries for other platforms.
Comment 22 ilia 2017-07-03 14:07:12 UTC
alsimon reported this deadlock issue on Solaris x86_64

BITNESS=64
CPUFAMILY=x86
CPUNUM=8
CPUTYPE=i386
HOSTNAME=beta
OSNAME=SunOS
OSBUILD=Oracle Solaris 11.3 X86
OSFAMILY=SUNOS
USER=alex
SH=/usr/bin/bash
USERDIRBASE=/export/home/alex
TMPDIRBASE=/var/tmp/dlight_alex/
DATETIME=2017-07-03 13:50:34
ENVFILE=/var/tmp/dlight_alex//env
ID=uid=205739(alex) gid=10(staff)
Comment 23 ilia 2017-07-03 14:07:47 UTC
Created attachment 164680 [details]
Thread Dump


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2014, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo