Consistent crashes under heavy load
Cellane opened this issue · 10 comments
The code below causes Vapor consistently with an error message of NIO-ELT-#3 (6): Assertion failed: illegal state: active: false, registered: true after about 6-7 seconds of running the wrk -t 8 -c 256 -d 30 http://localhost:8080/todos/latest command. When calling the method manually, all seems to be working fine and behaves as expected, it’s only crashing under heavy load. According to a short discussion on Vapor Discord, the code itself should hopefully be fine, so I assume the issue is somewhere in Redis implementation.
func latest(_ req: Request) throws -> Future<Todo> {
let redis = try req.make(RedisDatabase.self)
return redis.newConnection(on: req).flatMap { client in
return client
.jsonGet(TodoRedisKeys.latest.rawValue, as: Todo.self)
.unwrap(or: Abort(.notFound))
.catchFlatMap { _ in
return try Todo
.query(on: req)
.sort(\.id, .descending)
.first()
.unwrap(or: Abort(.notFound))
}
}
}Hi! Is there a reason you are not using a connection pool for this? Using a connection pool should smooth this out. I am using something like:
_ = req.withPooledConnection(to: .redis) { (redisClient: RedisClient) in
// do stuff here
}If you can, try that. I imagine the issue you are having is more appliance based then code based.
@pedantix The only reason I haven’t tried that is I couldn’t find that in the documentation 😆 I’ll give it a try tomorrow and come back with results, thanks!
Documentation is still an in-progress thing. Thanks for being an early adopter!
I there a reason why we need to be using a connection pool here? Is there an assumption that only one command can be sent at a time?
Seems like the withPooledConnection method works, and gives a much better performance, too! Still, it’s not without its flaws, there’s a lot of lines like these being printed into the console:
[ ERROR ] read(descriptor:pointer:size:) failed: Connection reset by peer (errno: 54) (Logger+LogError.swift:32)
[ DEBUG ] Conform `(String, String, String, UInt, UInt) -> ()` to `Debuggable` for better debug info. (Logger+LogError.swift:34)
I guess more pressing issue for me personally, though, is that there’s a method (newConnection(on:) from my original example) with a perfectly valid name, visible to the code we write, that’s happy to crash the entire server under heavy load. Even if documentation is updated to point developers at the withPooledConnection(to:) method, I don’t think it’s very user friendly to leave the method that can crash your server open to the public, unless there are use cases when you’d prefer newConnection(on:) over withPooledConnection(to:) – are there?
@Cellane you should only use newConnection(on:) for testing really. Anything that you expect to receive significant load should take advantage of connection pooling. Setting up new TCP connections is very expensive compared to making an actual Redis request.
That said, using newConnection(on:) should not crash. There must be a bug in how we are initializing Redis connections. I have some updates planned to streamline the NIO implementation here. I'll make sure this issue stays open until then.
Is this still a problem in 2020, especially with Redis 4?
yes
@gulivero1773 Can you provide some examples of how you're using Redis where you're seeing the crash, and any logs you may have from crashes?
Closing this issue due to inactivity. More information is needed in reports in order to understand the root cause.
If you are still using v3 and seeing crashes, please provide a snippet showing how you are using Redis where the crash is happening - with any logs you might have available.
If you are using v4, please provide the same information, but create the issue at RediStack.
Known issues / workarounds currently:
- If you are using the connection pool, you should be using
withPooledConnectioninstead ofnewConnection - As of Redis 3.4.0, a single RedisClient should be able to handle concurrent commands