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

[Yasumasa Suenaga]

On Sun, 4 Apr 2021 01:47:40 GMT, Chris Plummer <cjplummer at openjdk.org> wrote:

>>> 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?

Ok, I will change issue type to RFE, and will close both it and PR.

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

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