Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Intermittent failures in Shock v6 #34

Merged
merged 15 commits into from
Jun 28, 2021
Merged

Intermittent failures in Shock v6 #34

merged 15 commits into from
Jun 28, 2021

Conversation

antoniostrijdom
Copy link
Contributor

We were seeing intermittent failures on our CI system for Shock v6.

This PR:

  • Makes all setup data (routes) value types to improve thread safety.
  • Moves the middleware into an EventLoop.
  • Stops registering routes without a urlPath.
  • Drops Hashable routes in favour of an array of structs mapping routes to hander closures.
  • Fixes Equatable implementation (wasn't dealing with paths with wildcards correctly).
  • Tidies up some things.
  • Adds more tests.

@@ -26,12 +26,57 @@ class RouteTests: ShockTestCase {
}

func testSimpleRouteWithVariables() {
let route: MockHTTPRoute = .simple(method: .get, urlPath: "/simple/:foo", code: 200, filename: "testSimpleRoute.txt")
let route: MockHTTPRoute = .simple(method: .get, urlPath: "/simple/:foo", code: 200, filename: "testSimpleRouteWithVariables.txt")

Choose a reason for hiding this comment

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

Nice!

private func handleResponse(forResponseContext middlewareContext: MiddlewareContext, in channelHandlerContext: ChannelHandlerContext) {
private func handleResponse(forResponseContext middlewareContext: MiddlewareContext,
in channelHandlerContext: ChannelHandlerContext,
version: HTTPVersion) {

// TODO

Choose a reason for hiding this comment

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

It's not super clear what is To be done here? Should this comment be updated or even removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just formatting.

Choose a reason for hiding this comment

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

👍

responseFactory: self.responseFactory)
finalMiddleware.insert(routeMiddleware, at: 0)
}
let responder = MiddlwareResponder(middleware: finalMiddleware,
Copy link

@danlages danlages Jun 25, 2021

Choose a reason for hiding this comment

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

Just a suggestion

Suggested change
let responder = MiddlwareResponder(middleware: finalMiddleware,
let responder = MiddlwareResponder(middleware: finalMiddleware,

@danlages
Copy link

Really nice PR @antoniostrijdom - Super clean.

@albertodebortoli albertodebortoli merged commit 2ce5878 into justeat:master Jun 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants