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

fix(server/tls/streams) fix onReadFile, streams, avoid shutdown on fatal errors, ensure ssl loop data and server ref count refactor #12979

Merged
merged 64 commits into from
Aug 3, 2024

Conversation

cirospaciari
Copy link
Collaborator

@cirospaciari cirospaciari commented Jul 31, 2024

What does this PR do?

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

Existing tests

@robobun
Copy link

robobun commented Jul 31, 2024

@cirospaciari, your commit e01519d has 14 failures in #1256

@cirospaciari cirospaciari reopened this Jul 31, 2024
@cirospaciari cirospaciari marked this pull request as draft July 31, 2024 22:11
@cirospaciari cirospaciari changed the title fix(server) fix onReadFile fix(server) fix onReadFile and streams Jul 31, 2024
@cirospaciari cirospaciari changed the title fix(server) fix onReadFile and streams fix(server) fix onReadFile, streams and avoid shutdown on fatal errors Jul 31, 2024
@cirospaciari cirospaciari marked this pull request as ready for review July 31, 2024 23:45
@cirospaciari cirospaciari marked this pull request as draft August 1, 2024 00:20
@cirospaciari cirospaciari marked this pull request as ready for review August 1, 2024 01:53
@cirospaciari cirospaciari marked this pull request as draft August 1, 2024 21:11
@cirospaciari cirospaciari marked this pull request as ready for review August 2, 2024 05:15
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

2 comments but mostly LGTM

packages/bun-usockets/src/context.c Outdated Show resolved Hide resolved
packages/bun-usockets/src/socket.c Outdated Show resolved Hide resolved
@@ -1976,7 +1978,7 @@ fn NewRequestContext(comptime ssl_enabled: bool, comptime debug_mode: bool, comp
if (this.signal) |signal| {
this.signal = null;
if (this.flags.aborted and !signal.aborted()) {
const reason = JSC.WebCore.AbortSignal.createAbortError(JSC.ZigString.static("The user aborted a request"), &JSC.ZigString.Empty, this.server.globalThis);
const reason = JSC.WebCore.AbortSignal.createAbortError(JSC.ZigString.static("The user aborted a request"), &JSC.ZigString.Empty, globalThis);
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a follow-up, instead of creating this AbortError, we could probably add an enum to AbortSignal for known types of signals. Then, we can defer creating the JSValue here if it's not used.

Copy link
Contributor

github-actions bot commented Aug 3, 2024

@cirospaciari, clang-tidy had something to share with you about your code:

CMake Warning at CMakeLists.txt:267 (message):
  Expected LLVM 18 as the C++ compiler, build may fail or break at runtime.


  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0

  9  371M    9 36.2M    0     0  33.1M      0  0:00:11  0:00:01  0:00:10 33.1M
 29  371M   29  108M    0     0  51.6M      0  0:00:07  0:00:02  0:00:05 71.8M
 43  371M   43  162M    0     0  52.6M      0  0:00:07  0:00:03  0:00:04 63.2M
 62  371M   62  233M    0     0  57.0M      0  0:00:06  0:00:04  0:00:02 65.7M
 80  371M   80  300M    0     0  58.9M      0  0:00:06  0:00:05  0:00:01 65.9M
100  371M  100  371M    0     0  60.0M      0  0:00:06  0:00:06 --:--:-- 65.8M
100  371M  100  371M    0     0  60.0M      0  0:00:06  0:00:06 --:--:-- 64.4M

Commit: e01519d

@cirospaciari cirospaciari changed the title fix(server) fix onReadFile, streams and avoid shutdown on fatal errors fix(server/tls/streams) fix onReadFile, streams, avoid shutdown on fatal errors, ensure ssl loop data and server ref count refactor Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants