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

General warnings and tests cleanup #3107

Merged
merged 2 commits into from Nov 21, 2023
Merged

General warnings and tests cleanup #3107

merged 2 commits into from Nov 21, 2023

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Nov 21, 2023

These changes fix almost all of the extant warnings in Vapor, and clean up some issues in the tests, which can now run fully parallelized. There are no functional changes.

… and explicitly specify complete concurrency checking.
…lisions between tests when running tests in parallel, silence warnings when building with the nightlies.
@gwynne gwynne added the semver-patch Internal changes only label Nov 21, 2023
Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

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

There are still some warnings on linux, see the gql build log in CI.

Most of them are Date/Data warnings which I wouldn't bother fixing as Franz's changes should "take effect" sooner or later, but there are some NIOLockedValueBox warnings that can be resolved.

@@ -130,6 +130,7 @@ extension Request.Body: AsyncSequence {
failureType: Error.self,
backPressureStrategy: NIOAsyncSequenceProducerBackPressureStrategies
.HighLowWatermark(lowWatermark: 5, highWatermark: 20),
finishOnDeinit: true,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this fixes our crash on connection close issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Doubt it - finishOnDeinit: true was previously the default behavior anyway, they just force you to state it explicitly now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd've probably changed it up if I'd felt like digging into the appropriate semantics, but I had other things to figure out, as it happened 😆

@0xTim
Copy link
Member

0xTim commented Nov 21, 2023

@MahdiBM those should all be fixed in 5.9 - the only one that's left is the @Sendable function which requires the compiler change that was pitched to be implemented

@MahdiBM
Copy link
Contributor

MahdiBM commented Nov 21, 2023

O right, I was wondering why they aren't fixed yet. I forgot the gql CI stil uses 5.8 because 5.9 was having problems with gql.

@gwynne gwynne merged commit da9c280 into main Nov 21, 2023
16 checks passed
@gwynne gwynne deleted the warnings-cleanup branch November 21, 2023 17:09
@penny-for-vapor
Copy link

penny-for-vapor bot commented Nov 21, 2023

These changes are now available in 4.87.1

keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Jan 19, 2024
* main: (44 commits)
  Update routing-kit version (vapor#3131)
  Use `singleton` `EventLoopGroup` (vapor#3128)
  Additional tests fixes
  Fix for Apple changing things without warning.
  Add missing availability annotations in URI tests
  Merge pull request from GHSA-r6r4-5pr8-gjcp
  Fix setting public folder for `FileMiddleware` when using bundles (vapor#3113)
  Consistently use the value from `X-Request-Id` as the request's ID when present (vapor#3117)
  Fix encoding and decoding of HTTPHeaders (vapor#3116)
  Add fully async entrypoints (vapor#3114)
  Bring back AsyncCommands (vapor#3109)
  General warnings and tests cleanup (vapor#3107)
  Add public initializer for `XCTHTTPRequest` (vapor#3106)
  [skip ci] Update dependabot.yml
  [skip ci] Fixup sponsors workflow
  Make Async Request Body actually work (vapor#3096)
  Create a thread pool of System.coreCount rather than 64 when initializing an Application (vapor#3092)
  Modernize sponsors automation workflow (vapor#3098)
  Bump the dependencies group with 2 updates (vapor#3099)
  Update README with new Sponsor (vapor#3101)
  ...

# Conflicts:
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants