Apply suggestions for evpp dir from ai code review#838
Open
ithewei wants to merge 9 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a broad set of robustness and correctness improvements across libhv’s SSL backends, event-loop/watchers, and C++ utilities—primarily adding error handling, tightening thread/lifetime assumptions in evpp wrappers, and hardening parsing/utility helpers.
Changes:
- Added more defensive checks and error handling in SSL backends (OpenSSL/mbedTLS/GnuTLS) and in event-loop watcher backends (epoll/kqueue/evport/io_uring/iocp).
- Clarified evpp ownership/lifetime expectations and added in-loop thread assertions for loop-bound operations (TCP/UDP client/server wrappers).
- Hardened various base/cpputil utilities (URL parsing validation, thread-safety fixes, bounds checks, wait logic improvements, etc.).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ssl/openssl.c | Validate inputs and propagate SNI-setting failures rather than always returning success. |
| ssl/mbedtls.c | Add error handling for seed/setup, fix cleanup path, and ensure allocated SSL objects are freed. |
| ssl/gnutls.c | Add return-code checks during session init and SNI setting; propagate failures. |
| evpp/UdpServer.h | Add lifecycle notes and enforce loop-thread execution for startRecv(). |
| evpp/UdpServer_test.cpp | Use heap allocation (shared_ptr) and adjust calls for safer lifetime during callbacks. |
| evpp/UdpClient.h | Add lifecycle notes and enforce loop-thread execution for startRecv(). |
| evpp/UdpClient_test.cpp | Use heap allocation (shared_ptr) and adjust lambda captures/calls accordingly. |
| evpp/TcpServer.h | Add lifecycle notes, enforce acceptor-loop thread assertions, and improve connection bookkeeping/comments. |
| evpp/TcpClient.h | Add lifecycle notes, add reconnect-timer cancellation tracking, and assert loop-thread for connect/reconnect. |
| evpp/TcpClient_test.cpp | Use heap allocation (shared_ptr) and update usage patterns accordingly. |
| evpp/EventLoopThread.h | Add clarifying class-level comment about ownership/threading. |
| evpp/EventLoop.h | Add lifecycle notes and guard setTimerInLoop() against a NULL loop_. |
| evpp/Channel.h | Add comment clarifying loop-bound lifetime constraints for hio callbacks. |
| event/unpack.c | Replace debug-only assertions with runtime validation and error/close on invalid lengths. |
| event/select.c | Fix event counting to increment once per fd with any revents. |
| event/nio.c | Improve SSL handshake event registration/removal (handle WANT_WRITE and clear both read/write on success). |
| event/kqueue.c | Add logging include, validate init, add error handling for kevent operations, handle EINTR in poll. |
| event/iocp.c | Add logging include and validate IOCP creation failure. |
| event/io_uring.c | Add logging include and log io_uring_queue_init failures. |
| event/hloop.c | Improve watcher error logging; refine attach readbuf behavior; add robust error handling around add/del events. |
| event/evport.c | Add logging include, validate init, handle EINTR/errors in port_getn, and simplify io==NULL handling. |
| event/epoll.c | Add logging/socket errno include, validate init, and propagate epoll_ctl failures. |
| cpputil/ThreadLocalStorage.cpp | Add bounds checks for TLS indices and make threadName() thread-local to avoid data races. |
| cpputil/singleton.h | Replace call_once-based init with mutex-guarded init enabling re-creation after exitInstance(). |
| cpputil/RAII.cpp | Add stdio include and improve Win dump file creation/write/close error handling. |
| cpputil/LRUCache.h | Refactor eviction callback execution to occur outside locks; adjust eviction flows and include . |
| cpputil/ifconfig.cpp | Skip entries with NULL ifa_addr to avoid dereference crashes. |
| cpputil/hurl.cpp | Harden unescape() to avoid reading past end on malformed percent-escapes. |
| cpputil/hthreadpool.h | Replace busy-wait in wait() with condition-variable based waiting; track active tasks. |
| cpputil/hstring.cpp | Add guard checks for small strings and all-trim cases to avoid edge-case issues. |
| cpputil/hscope.h | Make ScopeCleanup destructor noexcept and guard against exceptions escaping cleanup. |
| cpputil/hpath.cpp | Fix islink() to check lstat return, and handle join() with empty dir. |
| cpputil/hobjectpool.h | Use predicate-based wait_for and correct locking patterns around factory creation. |
| cpputil/hfile.h | Update internal filepath on successful rename and harden readrange() bounds handling. |
| cpputil/hdir.cpp | Validate empty dir input and fix FindFirstFileW INVALID_HANDLE_VALUE check. |
| base/queue.h | Factor out queue realignment into helper to reduce duplication and improve clarity. |
| base/htime.h | Introduce hv_localtime_r/hv_gmtime_r wrappers for portable thread-safe time conversion. |
| base/htime.c | Use hv_localtime_r/hv_gmtime_r and avoid non-thread-safe localtime/gmtime usage. |
| base/hmath.h | Fix ASN.1 encoding threshold constant. |
| base/hlog.c | Improve gmtoff calculation and switch to thread-safe time conversions; adjust logging time code paths. |
| base/hdef.h | Fix MAKE_FOURCC macro to use uint32_t casts. |
| base/hbase.h | Fix a misleading comment related to hv_strncat usage. |
| base/hbase.c | Harden URL port parsing (digits/range) and propagate errors via return value. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+188
to
+190
| if (reconn_timer_id == timerID) { | ||
| reconn_timer_id = INVALID_TIMER_ID; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.