mirror of
https://github.com/Mezeporta/Erupe.git
synced 2026-03-21 23:22:34 +01:00
fix(channelserver): send ACK on empty Raviente register payload
handleMsgSysOperateRegister returned without sending an ACK when the payload was empty, causing the client to softlock waiting for a response. Send doAckBufSucceed with nil data on the early-return path to match the success-path ACK type. Also update tests to expect error returns instead of panics from unimplemented Build/Parse stubs (matching prior panic→error refactor), and mark resolved anti-patterns in docs.
This commit is contained in:
@@ -102,11 +102,9 @@ The original `mhfo-hd.dll` client reads the `ErrorCode` byte from `MsgSysAck` an
|
|||||||
|
|
||||||
### Scope
|
### Scope
|
||||||
|
|
||||||
A preliminary grep for `logger.Error` followed by bare `return` (no doAck call) found instances across ~25 handler files. The worst offenders are `handlers_festa.go`, `handlers_gacha.go`, `handlers_cafe.go`, and `handlers_house.go`. However, many of these are Pattern B (log-and-continue), not Pattern A. Each instance needs individual review to determine whether an ACK is already sent further down the function.
|
A preliminary grep for `logger.Error` followed by bare `return` (no doAck call) found instances across ~25 handler files. However, a thorough manual audit (2026-02-20) revealed that the vast majority are Pattern B (log-and-continue to a success ACK with empty data) or Pattern C (explicit fail ACK). Only one true Pattern A instance was found, in `handleMsgSysOperateRegister` (`handlers_register.go`), which has been fixed.
|
||||||
|
|
||||||
**Impact:** Players experience softlocks on error paths that could instead show an error dialog and let them continue playing.
|
**Status:** ~~Players experience softlocks on error paths.~~ **Fixed.** The last Pattern A instance (`handlers_register.go:62`) now sends `doAckBufSucceed` with nil data before returning. The ~87 existing `doAckSimpleFail`/`doAckBufFail` calls and the helper functions (`loadCharacterData`, `saveCharacterData`, `stubEnumerateNoResults`) provide comprehensive ACK coverage across all handler error paths.
|
||||||
|
|
||||||
**Recommendation:** Audit each silent-return error path. For handlers where the packet has an `AckHandle` and no ACK is sent on the error path, add `doAckSimpleFail`/`doAckBufFail` matching the ACK type used on the success path. This matches the existing pattern used in ~70 other error paths across the codebase.
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -312,11 +310,9 @@ func init() {
|
|||||||
|
|
||||||
## 11. Panic-Based Flow in Some Paths
|
## 11. Panic-Based Flow in Some Paths
|
||||||
|
|
||||||
Some error paths use `panic()` or `log.Fatal()` (which calls `os.Exit`) instead of returning errors, particularly in initialization and configuration code.
|
~~Some error paths use `panic()` or `log.Fatal()` (which calls `os.Exit`) instead of returning errors.~~ **Substantially fixed.** The 5 production `panic()` calls (4 in mhfpacket `Build()` stubs, 1 in binpacket `Parse()`) have been replaced with `fmt.Errorf` returns. The `byteframe.go` read-overflow panic has been replaced with a sticky error pattern (`ByteFrame.Err()`), and the packet dispatch loop in `sys_session.go` now checks `bf.Err()` after parsing to reject malformed packets cleanly.
|
||||||
|
|
||||||
**Impact:** Prevents graceful shutdown. Makes the server harder to embed in tests. Crashes the entire process instead of allowing recovery.
|
**Remaining:** The `recover()` in `handlePacketGroup` is retained as a safety net for any future unexpected panics.
|
||||||
|
|
||||||
**Recommendation:** Replace `panic`/`log.Fatal` with error returns. Reserve `panic` for truly unrecoverable programmer errors (e.g., invalid constants in `init`).
|
|
||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
@@ -353,9 +349,9 @@ Database operations use raw `database/sql` with PostgreSQL-specific syntax throu
|
|||||||
|
|
||||||
| Severity | Anti-patterns |
|
| Severity | Anti-patterns |
|
||||||
|----------|--------------|
|
|----------|--------------|
|
||||||
| **High** | Missing ACK responses / softlocks (#2), no architectural layering (#3), tight DB coupling (#13) |
|
| **High** | ~~Missing ACK responses / softlocks (#2)~~ **Fixed**, no architectural layering (#3), tight DB coupling (#13) |
|
||||||
| **Medium** | Magic numbers (#4), inconsistent binary I/O (#5), Session god object (#6), copy-paste handlers (#8), raw SQL duplication (#9) |
|
| **Medium** | Magic numbers (#4), inconsistent binary I/O (#5), Session god object (#6), copy-paste handlers (#8), raw SQL duplication (#9) |
|
||||||
| **Low** | God files (#1), `init()` registration (#10), inconsistent logging (#12), mutex granularity (#7), panic-based flow (#11) |
|
| **Low** | God files (#1), ~~`init()` registration (#10)~~ **Fixed**, ~~inconsistent logging (#12)~~ **Fixed**, mutex granularity (#7), ~~panic-based flow (#11)~~ **Fixed** |
|
||||||
|
|
||||||
### Root Cause
|
### Root Cause
|
||||||
|
|
||||||
|
|||||||
@@ -400,16 +400,13 @@ func TestMsgBinChatAllTypes(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestMsgBinMailNotifyParsePanics(t *testing.T) {
|
func TestMsgBinMailNotifyParseReturnsError(t *testing.T) {
|
||||||
defer func() {
|
|
||||||
if r := recover(); r == nil {
|
|
||||||
t.Error("Parse() should panic with 'implement me'")
|
|
||||||
}
|
|
||||||
}()
|
|
||||||
|
|
||||||
m := MsgBinMailNotify{}
|
m := MsgBinMailNotify{}
|
||||||
bf := byteframe.NewByteFrame()
|
bf := byteframe.NewByteFrame()
|
||||||
_ = m.Parse(bf)
|
err := m.Parse(bf)
|
||||||
|
if err == nil {
|
||||||
|
t.Error("Parse() should return an error (not implemented)")
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestMsgBinMailNotifyBuildLongName(t *testing.T) {
|
func TestMsgBinMailNotifyBuildLongName(t *testing.T) {
|
||||||
|
|||||||
@@ -102,19 +102,15 @@ func TestMsgBinMailNotify_Build(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestMsgBinMailNotify_Parse_Panics(t *testing.T) {
|
func TestMsgBinMailNotify_Parse_ReturnsError(t *testing.T) {
|
||||||
// Document that Parse() is not implemented and panics
|
// Document that Parse() is not implemented and returns an error
|
||||||
msg := MsgBinMailNotify{}
|
msg := MsgBinMailNotify{}
|
||||||
bf := byteframe.NewByteFrame()
|
bf := byteframe.NewByteFrame()
|
||||||
|
|
||||||
defer func() {
|
err := msg.Parse(bf)
|
||||||
if r := recover(); r == nil {
|
if err == nil {
|
||||||
t.Error("Parse() did not panic, but should panic with 'implement me'")
|
t.Error("Parse() should return an error (not implemented)")
|
||||||
}
|
}
|
||||||
}()
|
|
||||||
|
|
||||||
// This should panic
|
|
||||||
_ = msg.Parse(bf)
|
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestMsgBinMailNotify_BuildMultiple(t *testing.T) {
|
func TestMsgBinMailNotify_BuildMultiple(t *testing.T) {
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
package mhfpacket
|
package mhfpacket
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"erupe-ce/common/byteframe"
|
"erupe-ce/common/byteframe"
|
||||||
@@ -261,7 +262,8 @@ func TestBuildCoverage_NotImplemented(t *testing.T) {
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
// Build returned an error, which is expected for NOT IMPLEMENTED stubs
|
// Build returned an error, which is expected for NOT IMPLEMENTED stubs
|
||||||
if err.Error() != "NOT IMPLEMENTED" {
|
errMsg := err.Error()
|
||||||
|
if errMsg != "NOT IMPLEMENTED" && !strings.Contains(errMsg, "not implemented") {
|
||||||
t.Errorf("Build() returned unexpected error: %v", err)
|
t.Errorf("Build() returned unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -59,6 +59,7 @@ func handleMsgSysOperateRegister(s *Session, p mhfpacket.MHFPacket) {
|
|||||||
pkt := p.(*mhfpacket.MsgSysOperateRegister)
|
pkt := p.(*mhfpacket.MsgSysOperateRegister)
|
||||||
|
|
||||||
if len(pkt.RawDataPayload) == 0 {
|
if len(pkt.RawDataPayload) == 0 {
|
||||||
|
doAckBufSucceed(s, pkt.AckHandle, nil)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user