Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a method for user programs to be notified when a worker thread is started or stopped #1126

Closed
wants to merge 3 commits into from

Conversation

ThadHouse
Copy link
Contributor

For some languages, they need a way to be notified of worker threads that will call back into user code (specifically Java). They need to be notified both on starting and stopping. This PR adds a new callback that can be set to allow users to know.

Testing still needs to be added, but wanted to double check the API.

… started or stopped

For some languages, they need a way to be notified of worker threads that will call back into user code (specifically Java). They need to be notified both on starting and stopping. This PR adds a new callback that can be set to allow users to know.
@ThadHouse ThadHouse added the Area: Core Related to the shared, core protocol logic label Dec 28, 2020
@ThadHouse ThadHouse requested a review from a team as a code owner December 28, 2020 19:19
@nibanks
Copy link
Member

nibanks commented Dec 28, 2020

I'm still not really a big fan of exposing this. If at all possible, I prefer it scope this to the abstraction layer instead. Can we put this on the back burner and discuss more next week?

@nibanks
Copy link
Member

nibanks commented Dec 28, 2020

For one, if multiple components in the same process are trying to use the same msquic library, with this model, they can step on each others' toes. I really prefer a language specific version of msquic that wouldn't result in that type of issue (because they wouldn't be shared).

@ThadHouse
Copy link
Contributor Author

Yeah we can figure it out next week.

@ThadHouse
Copy link
Contributor Author

ThadHouse commented Dec 29, 2020

One thing I realized while going through the code is that workers are created in registration. I thought they were created in the library. Because of this, we could actually add them as another field to QUIC_REGISTRATION_CONFIG, and then it would be scoped to the registration, although that is a binary breaking change. Realize we should have passed in a size for that config.

Also, building a separate copy of the library doesn't actually solve the same process sharing issues, you'd run into the same things either way, unless we passed in through the registration.

@ThadHouse
Copy link
Contributor Author

ThadHouse commented Dec 29, 2020

Thats a lot better, and much more scoped in too. It is more of a breaking API change then the previous idea however. I'll write up a small demo app to show how this would actually work with the thread locals. IIRC, .NET can also use functionality like this.


typedef struct QUIC_REGISTRATION_CONFIG { // All fields may be NULL/zero.
const char* AppName;
QUIC_EXECUTION_PROFILE ExecutionProfile;
QUIC_WORKER_CALLBACK_HANDLER WorkerStateHandler;
Copy link
Member

Choose a reason for hiding this comment

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

You can make this non-breaking by putting it behind a "custom" execution profile flag. Though, still I don't believe it should be architected this way.

nibanks referenced this pull request in wkgcass/msquic-java Dec 31, 2020
@wkgcass
Copy link

wkgcass commented Jan 1, 2021

Based on this approach, in my opinion, an improvement could be made:

The new field in QUIC_REGISTRATION_CONFIG could be changed into a struct and allow it to store some user data.
Something like:

struct QUIC_WORKER_CONFIG {
    void WorkerStateHandler(QUIC_WORKER_EVENT_TYPE EventType, void* UserData);
    void* UserData; // will be passed into the 'WorkerStateHandler' function
} QUIC_WORKER_CONFIG;

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

    QUIC_WORKER_CONFIG WorkerConfig;
} QUIC_REGISTRATION_CONFIG;

In this way, users will be able to pass in JavaVM* into UserData, and directly use the UserData pointer in the callback function instead of using a global variable.
This also allows users to use different variables for different registrations.

@nibanks
Copy link
Member

nibanks commented Jan 1, 2021

Another problem with the current approach is that it is only for the app's specific registration threads. It isn't applied for the shared listener threads.

@ThadHouse
Copy link
Contributor Author

Replaced by #1142

@ThadHouse ThadHouse closed this Jan 6, 2021
@ThadHouse ThadHouse deleted the thadhouse/workerinitializecallback branch January 6, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Core Related to the shared, core protocol logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants