RFR: 8264639: SA: Worker thread for ptrace on Linux should be more robustness

[Chris Plummer]

On Sat, 3 Apr 2021 12:26:33 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

>> My first reaction to these changes is that they are fairly extensive, will take a lot of reviewing time, and don't seem to actually fix something that in real life is an issue. So I'm unsure of your motivation for all the work put into this issue.
>
>> I'm unsure of your motivation for all the work put into this issue.
> 
> When I modified LinuxDebuggerLocal to add ptrace accessor, I couldn't understand why the method should be qualified `synchronized`. ptrace caller should only one thread, so I understand the need of worker thread.
> The comment of WorkerThreadTask says the need of worker thread, but it does not mention the caller should be synchronized.
> 
> We can just add the comment of course, but we can simplify the worker with modern API. We might add new worker tasks in future, so I thought it's better to refactor in this opportunity.
> 
> For example, following the current approach, we need to add new worker task in below:
> 
> class CreateDwarfParserTask implements WorkerThreadTask {
>     DwarfParser result;
>     public void doit(LinuxDebuggerLocal debugger) {
>         result = new DwarfParser(libptr, rip, rbp, rsp, debugger);
>     }
> }
> 
> CreateDwarfParserTask task = new CreateDwarfParserTask();
> workerThread.execute(task);
> return task.result;
> 
> But we can simplify it after this PR:
> 
> try {
>     return ptraceWorkerThreadPool.submit(() -> new DwarfParser(libptr, rip, rbp, rsp, LinuxDebuggerLocal.this))
>                                  .get();
> } catch (InterruptedException | ExecutionException e) {
>     throw new DebuggerException(e);
> }
> 
> This PR will take a lot of reviewing time as you said. If it does not worth to work, I want to add the comment about `synchronized` at least.

Can't this issue also be fixed by simply making `execute()` synchronized? Then you don't need to worry about the caller being synchronized.

My suggestion would to not make this change at this point, since it's not addressing an actual bug the user can run into. You can add documentation in the code, which can also reference the CR. The CR should be changed to an RFE, and for now closed as "Will Not Fix". You can archive a reference to your current change from the CR in case there ever becomes a real need to address this. Does that sound ok?

-------------

PR: https://git.openjdk.java.net/jdk/pull/3321