access field handle of FileDescriptor in ProcessImplForWin32 by reflection for portability#2113
access field handle of FileDescriptor in ProcessImplForWin32 by reflection for portability#2113davidzollo merged 3 commits intoapache:devfrom
Conversation
| FD_ACCESS = FileDescriptor.class.getDeclaredField("fd"); | ||
| FD_ACCESS.setAccessible(true); | ||
| } catch (NoSuchFieldException e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
I'm unfamiliar with exception strategy in DolphinScheduler so it would be better our committers can check whether the exception thrown obeys conventions.
| try { | ||
| FD_ACCESS.set(obj, fd); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException(e); |
| try { | ||
| return (Long) FD_ACCESS.get(obj); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException(e); |
Codecov Report
@@ Coverage Diff @@
## dev #2113 +/- ##
======================================
Coverage ? 29.64%
Complexity ? 1607
======================================
Files ? 336
Lines ? 17833
Branches ? 2250
======================================
Hits ? 5287
Misses ? 11990
Partials ? 556
Continue to review full report at Codecov.
|
|
This PR contains only refactor changes, I don't think any further tests should be added. Please take a look. |
|
@liwenhe1993 could you share the error message and check that it is running with this patch? I cannot reproduce and it seems our ci system doesn't complain about it or report something helps. |
|
…ction for portability Current implementation relies on `sun.misc.JavaIOFileDescriptorAccess` which is only accessible on oraclejdk8. Basically the demand is getting & setting `handle` field of `FileDescriptor`, so we can directly do that with reflection. Though, I suspect the necessity we introduce ProcessImplForWin32. Maybe we could have a better way to support worker server to run bat script.
|
@liwenhe1993 thanks for your information. It seems I misspell the field name in the patch. Could you please verify the latest patch? |
|
Yeah, I will do, but UT run failed. Maybe ocurred error by: static {
try {
FD_HANDLE = FileDescriptor.class.getDeclaredField("handle");
FD_HANDLE.setAccessible(true);
} catch (NoSuchFieldException e) {
throw new RuntimeException(e);
}
}So, UT should be updated. |
|
@liwenhe1993 Thanks for your updated. I don't have a debug environment now. It is hard to debug with single NPE message, could you checkout the patch and fix the issue? The reflection direction looks viable and I think the authority of the patch could then belong to you :-) |
|
Code: /**
* Method: handle() for Windows
*/
@Test
public void testHandleForWindows() throws Exception {
try {
PowerMockito.when(OSUtils.isWindows()).thenReturn(true);
shellTask.handle();
Assert.assertTrue(true);
} catch (Error | Exception e) {
// if (!e.getMessage().contains("process error . exitCode is : -1")) {
logger.error(e.getMessage());
// }
}
}UT can run successfully when you updated this file. |
|
SonarCloud Quality Gate failed.
|
|
good job thanks for your contribution, looking forward to your next contribution |
What is the purpose of the pull request
Current implementation relies on
sun.misc.JavaIOFileDescriptorAccesswhich is only accessible on oraclejdk8.
Basically the demand is getting & setting
handlefield ofFileDescriptor,so we can directly do that with reflection.
Though, I suspect the necessity we introduce ProcessImplForWin32. Maybe
we could have a better way to support worker server to run bat script.
Brief change log
Replace
JavaIOFileDescriptorAccesswith reflections toFileDescriptor.Verify this pull request
This pull request is already covered by existing tests, such as
ProcessImplForWin32TestThis closes #2111 .