Skip to content

Commit

Permalink
add jni related functions to msquic
Browse files Browse the repository at this point in the history
  • Loading branch information
@wkgcass
wkgcass committed Dec 24, 2020
1 parent e449e47 commit 3e87aa1
Show file tree
Hide file tree
Showing 5 changed files with 173 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/bin/linux/exports.txt
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
msquic
{
global: MsQuicOpen; MsQuicClose;
global: MsQuicOpen; MsQuicClose; MsQuicJavaInit; MsQuicGetJNIEnv;
local: *;
};
54 changes: 54 additions & 0 deletions src/core/library.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,60 @@ QuicLibraryGetParam(
return Status;
}

// actual variables are defined in platform_linux.c
// because the tools will use the platform lib without the core lib
extern pthread_key_t ThreadLocalJNIEnvKey;
extern void* GlobalJvm; // JavaVM*
extern void* (* AttachThreadFunc)(void*); // JavaVM* => JNIEnv*
extern void (* DetachThreadFunc)(void*); // JavaVM* => void

_IRQL_requires_max_(PASSIVE_LEVEL)
void
QUIC_API
MsQuicJavaInit(
_In_ void* Jvm, // JavaVM*
_In_ ATTACH_THREAD_FUNC AttachThreadFuncLocal,
_In_ DETACH_THREAD_FUNC DetachThreadFuncLocal
)
{
int err = pthread_key_create(&ThreadLocalJNIEnvKey, NULL);
if (err < 0) {
QuicTraceEvent(
JavaError,
"[java] Error: %u %s.",
errno,
"create pthread_key failed");
}
GlobalJvm = Jvm;
AttachThreadFunc = AttachThreadFuncLocal;
DetachThreadFunc = DetachThreadFuncLocal;
}

_IRQL_requires_max_(PASSIVE_LEVEL)
QUIC_STATUS
QUIC_API
MsQuicGetJNIEnv(
_Out_ void** OutEnv // &JNIEnv*
)
{
void* env = pthread_getspecific(ThreadLocalJNIEnvKey);
if (env == NULL) {
if (GlobalJvm == NULL) {
QuicTraceEvent(
SimpleJavaError,
"[java] Error: %s.",
"GlobalJvm not set");
}
QuicTraceEvent(
SimpleJavaError,
"[java] Error: %s.",
"ThreadLocalJNIEnv not set");
return QUIC_STATUS_INTERNAL_ERROR;
}
*OutEnv = env;
return QUIC_STATUS_SUCCESS;
}

_IRQL_requires_max_(PASSIVE_LEVEL)
QUIC_STATUS
QUIC_API
Expand Down
29 changes: 29 additions & 0 deletions src/inc/msquic.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1154,6 +1154,35 @@ MsQuicClose(
_In_ _Pre_defensive_ const QUIC_API_TABLE* QuicApi
);

//
// JVM
//

typedef void* (* ATTACH_THREAD_FUNC)(void*); // JavaVM* => JNIEnv*
typedef void (* DETACH_THREAD_FUNC)(void*); // JavaVM* => void

//
// Initiating Java environment
//
_IRQL_requires_max_(PASSIVE_LEVEL)
void
QUIC_API
MsQuicJavaInit(
_In_ void* Jvm, // JavaVM*
_In_ ATTACH_THREAD_FUNC AttachThreadFunc,
_In_ DETACH_THREAD_FUNC DetachThreadFunc
);

//
// Retrieving the JNIEnv* for the current thread
//
_IRQL_requires_max_(PASSIVE_LEVEL)
QUIC_STATUS
QUIC_API
MsQuicGetJNIEnv(
_Out_ void** OutEnv // &JNIEnv*
);

#if defined(__cplusplus)
}
#endif
Expand Down
36 changes: 36 additions & 0 deletions src/manifest/clog.sidecar
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9525,6 +9525,34 @@
}
],
"macroName": "QuicTraceLogInfo"
},
"JavaError": {
"ModuleProperites": {},
"TraceString": "[java] Error: %u %s.",
"UniqueId": "JavaError",
"splitArgs": [
{
"DefinationEncoding": "u",
"MacroVariableName": "arg2"
},
{
"DefinationEncoding": "s",
"MacroVariableName": "arg3"
}
],
"macroName": "QuicTraceEvent"
},
"SimpleJavaError": {
"ModuleProperites": {},
"TraceString": "[java] Error: %s.",
"UniqueId": "SimpleJavaError",
"splitArgs": [
{
"DefinationEncoding": "s",
"MacroVariableName": "arg2"
}
],
"macroName": "QuicTraceEvent"
}
},
"Version": 1,
Expand Down Expand Up @@ -12374,6 +12402,14 @@
{
"UniquenessHash": "7368812e-a46d-0924-7643-009511288886",
"TraceID": "InteropTestStop"
},
{
"UniquenessHash": "7ca9a627-ea0b-4a7a-76b5-02bd899e607a",
"TraceID": "JavaError"
},
{
"UniquenessHash": "6f7ab853-c29e-8843-88f2-56a8aff0aeab",
"TraceID": "SimpleJavaError"
}
]
}
Expand Down
54 changes: 53 additions & 1 deletion src/platform/platform_linux.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -687,6 +687,55 @@ QuicConvertFromMappedV6(
}
}

// actual variables here because the tools will use the platform lib without the core lib
pthread_key_t ThreadLocalJNIEnvKey;
void* GlobalJvm = NULL; // JavaVM*
void* (* AttachThreadFunc)(void*); // JavaVM* => JNIEnv*
void (* DetachThreadFunc)(void*); // JavaVM* => void

typedef struct st_quic_thread_create_ctx {
void* Context;
LPTHREAD_START_ROUTINE Callback;
} quic_thread_create_ctx_t;

void* quic_thread_runnable(void* data) {
quic_thread_create_ctx_t* ctx = data;
void* Context = ctx->Context;
LPTHREAD_START_ROUTINE Callback = ctx->Callback;
free(ctx);

if (GlobalJvm == NULL) {
QuicTraceEvent(
SimpleJavaError,
"[java] Error: %s.",
"GlobalJvm not set");
goto quic_thread_runnable_err;
}
void* env = AttachThreadFunc(GlobalJvm);
if (env == NULL) {
QuicTraceEvent(
SimpleJavaError,
"[java] Error: %s.",
"attaching thread failed");
goto quic_thread_runnable_err;
}
int err = pthread_setspecific(ThreadLocalJNIEnvKey, env);
if (err < 0) {
QuicTraceEvent(
JavaError,
"[java] Error: %u %s.",
errno,
"setting jni env to thread local failed");
DetachThreadFunc(GlobalJvm);
goto quic_thread_runnable_err;
}
void* ret = Callback(Context);
DetachThreadFunc(GlobalJvm);
return ret;
quic_thread_runnable_err:
return Callback(Context);
}

QUIC_STATUS
QuicThreadCreate(
_In_ QUIC_THREAD_CONFIG* Config,
Expand Down Expand Up @@ -734,7 +783,10 @@ QuicThreadCreate(
}
}

if (pthread_create(Thread, &Attr, Config->Callback, Config->Context)) {
quic_thread_create_ctx_t* quic_thread_create_ctx = malloc(sizeof(quic_thread_create_ctx_t));
quic_thread_create_ctx->Callback = Config->Callback;
quic_thread_create_ctx->Context = Config->Context;
if (pthread_create(Thread, &Attr, quic_thread_runnable, quic_thread_create_ctx)) {
Status = errno;
QuicTraceEvent(
LibraryErrorStatus,
Expand Down

12 comments on commit 3e87aa1

@nibanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wkgcass I noticed you making these changes and I was wondering what your end goal was. Do you plan to try to merge any of this back into msquic mainline? cc @ThadHouse

@wkgcass
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nibanks Hi, MsQuic is great work. I would like to use MsQuic in Java through JNI, and use it in my networking toolkit called vproxy.
This commit basically adds two apis: one allow JNI code to pass JavaVM* into the lib, another retrieving JNIEnv* for the current thread, which will be used in the listener/connection/stream callbacks. And when threads launch, attach the spawn threads to JVM and store the JNIEnv* to threadlocal.
This commit is a little tricky and seems only apply for this specific usage, so I'm afraid this might not be able to merge to the origin.
However this commit is kept small and easy to maintain.

@wkgcass
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm believe a more general way of implementing this is to add a callback function which triggers when thread starts/stops.
And would be better to have a thread related context be tracked and passed to the user callbacks to reduce the cost of using thread local variables.

@ThadHouse
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first part I'm looking into, at least for the specific threads that enter Java. This only changes a small number of lines to do so.

The 2nd part is actually a much bigger change. Its something I can see the usability in wanting it, but doing so is fairly invasive, especially because a connection/listener/stream isn't held to a specific worker over its lifetime. Thread locals for that are likely easier and cleaner, especially with C having built in thread locals nowadays.

@nibanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference to fix the first part is something like this. Then the Java code would define something like platform_linux_java.c, which would just implement QuicThreadCustomStart to do what it needs. Then a new CMake arg/option would be for "Java Mode" which would define QUIC_USE_CUSTOM_THREAD_CONTEXT and include the new platform_linux_java.c file. This would allow good separation (IMO) between the generic and Java-specific code. I don't want to expose this via the official MsQuic API because this is still platform specific logic and not application layer, again, IMO.

As far as the second, like @ThadHouse said, that would be a MUCH bigger change, and I'd really try to avoid that if at all possible. We'd have to have proof that the change was absolutely necessary before I'd consider it.

@wkgcass
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can see that the second part is nearly impossible to implement for the current code base, and using a thread local variable solves most problems and the thread local performance cost is acceptable.

@wkgcass
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your example code. @nibanks

I'm wondering if we can make the QuicThreadCustomStart a standard api in this way:

1. define thread related parameters in QUIC_REGISTRATION_CONFIG
struct QUIC_THREAD_CONFIG {
    void OnThreadStarts(void* UserData); // will be called when a new thread starts
    void OnThreadEnds(void* UserData); // will be called before thread finishes
    void* UserData; // will be passed into the 'OnThreadStarts' and 'OnThreadEnds' functions
} QUIC_THREAD_CONFIG;

typedef struct QUIC_REGISTRATION_CONFIG {
    const char* AppName;
    QUIC_EXECUTION_PROFILE ExecutionProfile;

    QUIC_THREAD_CONFIG Thread;
} QUIC_REGISTRATION_CONFIG;
2. user code

Users can say:

void AttachToJavaVM(void* UserData) {
    JavaVM* JVM = (JavaVM*)UserData;
    // attach and store the JNIEnv* to a thread local variable
    // ...
}

void DetachFromJavaVM(void* UserData) {
    // detach ...
}

QUIC_REGISTRATION_CONFIG RegConf;
RegConf.Thread.OnThreadStarts = AttachToJavaVM;
RegConf.Thread.OnThreadEnds = DetachFromJavaVM;
RegConf.Thread.UserData = JVM;

I haven't read the Registration code (I was reading the Listeners/Connections/Streams), so I'm not sure whether the code snippets make sense.
I'll read the related code to verify my idea today or tomorrow.

@nibanks
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's possible and pretty much what @ThadHouse has in microsoft/msquic#1126. I am hesitant to go that route though, because it isn't something that most apps would actually need... BTW, instead of having this discussion here, @wkgcass would you mind starting a discussion about this here: https://github.com/microsoft/msquic/discussions?

@wkgcass
Copy link
Owner Author

@wkgcass wkgcass commented on 3e87aa1 Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's possible and pretty much what @ThadHouse has in microsoft/msquic#1126. I am hesitant to go that route though, because it isn't something that most apps would actually need... BTW, instead of having this discussion here, @wkgcass would you mind starting a discussion about this here: https://github.com/microsoft/msquic/discussions?

Maybe just discuss the related topics in the PR ?

@nibanks
Copy link

@nibanks nibanks commented on 3e87aa1 Jan 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've talked with my coworkers some more and we'd be interested in working with you and merging your work for Java support into mainline MsQuic. Would you be interested in pushing these changes back (with some changes most likely)? cc @anrossi & @ThadHouse

@wkgcass
Copy link
Owner Author

@wkgcass wkgcass commented on 3e87aa1 Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've talked with my coworkers some more and we'd be interested in working with you and merging your work for Java support into mainline MsQuic. Would you be interested in pushing these changes back (with some changes most likely)? cc @anrossi & @ThadHouse

I'm willing to. I'm sure a lot of modifications should be made... Any suggestion?

@nibanks
Copy link

@nibanks nibanks commented on 3e87aa1 Jan 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm willing to. I'm sure a lot of modifications should be made... Any suggestion?

I've opened microsoft/msquic#1141 to track adding Java support. We can move our discussion there. My goal would be to figure out how we can break what you have up into separate changes that are easier to design and code review. To that effect, I've created microsoft/msquic#1142. We can discuss more on either the issue or the PR. Thanks!

Please sign in to comment.