search

ethanmoffat / etheos

[Eth]an's [E]ndless [O]nline [S]erver - fork of eoserv project
zlib License
13 stars 8 forks source link

Add threadpool for queueing of background operations asynchronously #15

Closed ethanmoffat closed 4 years ago

ethanmoffat commented 4 years ago

Threadpool replaces the previous implementation of PaswordHashUpdater and encapsulates the background thread/signaling functionality that queued incoming password version updates. Now, the work is queued on the Threadpool and most of the logic from PasswordHashUpdater is handled by the Threadpool.

Number of threads in the pool defaults to number of CPUs on the system. This can be overridden by a config value but shall not exceed 32 (arbitrarily chosen).

Additionally a bug in semaphore was fixed that did not release the proper number of claims. Semaphore also now has a maximum count that is honored when releasing.

ethanmoffat commented 4 years ago

This change is in preparation for changes related to password hashing. The eventual goal is to have all hashing operations happen in the background. This will prevent lengthy hash operations (bcrypt with a lot of rounds) from blocking server execution. For instance, logins with bcrypt currently cause time deltas to be detected by eoserv since hashing is a long-running, blocking operation.

What I wouldn't give for async/await in C++...

ethanmoffat commented 4 years ago

I might have opened this prematurely. I'm going to try and get tests written for ThreadPool/semaphore since they should be pretty easy to test.

iandinwoodie commented 4 years ago

@ethanmoffat made some initial comments to start consuming the branch on my end. I'll be working through this one in chunks.

ethanmoffat commented 4 years ago

@ethanmoffat made some initial comments to start consuming the branch on my end. I'll be working through this one in chunks.

Thanks, sorry it's kinda big. The original intent was to do all password operations in one PR but that change was getting too large. I appreciate the reviews.

Also I just realized I have other unpushed changes. Not sure why I didn't push them. All changes are to CMake. The first ensures that debug builds properly define the DEBUG preprocessor macro on Windows (currently broken). The second links the handler files for commands and packet handlers directly to main. I'm seeing a weird issue with initialization logic that I'll detail in more depth on the file once I push.

ethanmoffat commented 4 years ago

Sorry for the delay in the review. It's been on my to-do list for several weeks now. Tried to be thorough without being needlessly nit-picky. Great looking patch and the unit tests you added are really awesome.

No worries. I really appreciate it. You're catching a lot of stuff I obviously missed, it's fantastic having a second set of eyes on this stuff.