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.

Sign up for GitHub

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

Jump to bottom

can lsquic support multi-thread? #167

Closed
jazune opened this issue Sep 24, 2020 · 13 comments
Closed

can lsquic support multi-thread? #167

jazune opened this issue Sep 24, 2020 · 13 comments

Comments

@jazune
Copy link

jazune commented Sep 24, 2020 •
edited

It seems that lsquic_hash is not thread safe. I got a crash in insert_compress_certs with more than one threads
@dtikhonov
Copy link
Contributor

That's true. This has been reported in #128.

@jazune
Copy link
Author

jazune commented Sep 25, 2020

Do you plan to support multi-thread ? It seems not just one problems in multi-thread。

@dtikhonov
Copy link
Contributor

dtikhonov commented Sep 25, 2020 •
edited

Well, we obviously do not use this in a multi-threaded environment, so we did not pay close attention to it. If it's not too much work, we can fix some trouble spots.

Also, "multi-thread support" means different things to different people. To me, it means being able to use two instances of lsquic engine in two different threads without them stepping on each other's toes.

Besides the compressed certs, what other problems have you encountered?

@dtikhonov
Copy link
Contributor

@jazune, I made a few fixes -- I believe the library is now thread-safe. Could you please give it a shot? The changes are on the 202009251152-thread-safety branch.

@jazune
Copy link
Author

jazune commented Sep 28, 2020

@jazune, I made a few fixes -- I believe the library is now thread-safe. Could you please give it a shot? The changes are on the 202009251152-thread-safety branch.
Great ! I will have a try.
It seems all global variables have the rrisk in multi thread.
Maybe these global variables can set to thread_local

@dtikhonov
Copy link
Contributor

Our preference would be to avoid thread-related code in the library if we can avoid it. Hopefully, just getting rid of global variables (as I've done on this branch) will do the trick.

litespeedtech pushed a commit that referenced this issue Oct 1, 2020
Fix several thread safety issues
2f4629f 
Fixes bug #128 and bug #167.
@litespeedtech
Copy link
Owner

Fixed -- closing.

litespeedtech pushed a commit that referenced this issue Oct 7, 2020
Release 2.22.0
fbc6cc0 
- [FEATURE] Extensible HTTP Priorities (HTTP/3 only).
- [FEATURE] Add conn context to packet-out memory interface (PR #175).
- [BUGFIX] gQUIC proof generation: allocate buffer big enough for
  signature (issue #173).
- [BUGFIX] Make library thread-safe: drop use of global variables
  (issue #133, issue #167).
- [BUGFIX] Deactivate only *recent* HQ frame, not any HQ frame.
- [BUGFIX] gQUIC server: associate compressed cert with SSL_CTX,
  instead of keeping them in a separate hash, potentially leading
  to mismatches.
- [BUGFIX] Stream data discard infinite loop: break on FIN.
- cmake: add install target via -DCMAKE_INSTALL_PREFIX (PR #171).
- Support randomized packet number to begin a connection.
- Mini and full IETF connection size optimization.
- http_client: specify HTTP priorities based on stream conditions.
@jazune
Copy link
Author

jazune commented Oct 9, 2020

@dtikhonov when will this branch merge to master?

@dtikhonov
Copy link
Contributor

It's on master -- the fix is in release 2.20.0.

@jazune
Copy link
Author

jazune commented Oct 10, 2020

Why are some variables in lsquic uninitialized? You take them all as initial zero. Won't there be any problems here? Of course，it has nothing to do with this subject.

@dtikhonov
Copy link
Contributor

Give me an example.

@jazune
Copy link
Author

jazune commented Oct 13, 2020 •
edited

eg：
static lsquic_server_config_t s_server_config;

if (s_server_config.lsc_scfg && (s_server_config.lsc_scfg->info.expy > (uint64_t)t))
    return &s_server_config;

s_server_config.lsc_scfg was uninitialized

@dtikhonov
Copy link
Contributor

That's old code now: use latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dtikhonov @litespeedtech @jazune