On Sun, 4 Apr 2021 01:47:40 GMT, Chris Plummer <[email protected]> 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
