Skip to content

Commit

Permalink
fix: correctly deallocate info buffers
Browse files Browse the repository at this point in the history
This ensures that if either of the infos was in fact built,
but the deallocate function is called before the respective
toPacket function, we release the allocated buffer.
This can happen due to exceptions or just abnormal process flow,
in any case, we make the assumption that if an info object is
deallocated, it will not be written into the channels from that
point onward, and as such, it is safe to release the buffers related
to them.
  • Loading branch information
Z-Kris committed May 3, 2024
1 parent 2076deb commit 5e5cfc2
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ public class NpcInfo internal constructor(
*/
internal var exception: Exception? = null

/**
* Whether the buffer allocated by this NPC info object has been built
* into a packet message. If this returns false, but NPC info was in fact built,
* we have an allocated buffer that needs releasing. If the NPC info itself
* is released but isn't built into packet, we make sure to release it, to avoid
* any memory leaks.
*/
private var builtIntoPacket: Boolean = false

/**
* Returns the backing byte buffer holding all the computed information.
* @throws IllegalStateException if the buffer is null, meaning it has no yet been
Expand All @@ -152,6 +161,7 @@ public class NpcInfo internal constructor(
exception,
)
}
builtIntoPacket = true
return if (this.viewDistance > MAX_SMALL_PACKET_DISTANCE) {
NpcInfoLarge(backingBuffer())
} else {
Expand All @@ -167,6 +177,7 @@ public class NpcInfo internal constructor(
// Acquire a new buffer with each cycle, in case the previous one isn't fully written out yet
val buffer = allocator.buffer(BUF_CAPACITY, BUF_CAPACITY)
this.buffer = buffer
this.builtIntoPacket = false
return buffer
}

Expand Down Expand Up @@ -505,9 +516,11 @@ public class NpcInfo internal constructor(
}

override fun onDealloc() {
val buffer = this.buffer
if (buffer != null && buffer.refCnt() > 0) {
buffer.release(buffer.refCnt())
if (!builtIntoPacket) {
val buffer = this.buffer
if (buffer != null && buffer.refCnt() > 0) {
buffer.release(buffer.refCnt())
}
}
this.buffer = null
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,15 @@ public class PlayerInfo internal constructor(
*/
internal var exception: Exception? = null

/**
* Whether the buffer allocated by this player info object has been built
* into a packet message. If this returns false, but player info was in fact built,
* we have an allocated buffer that needs releasing. If the NPC info itself
* is released but isn't built into packet, we make sure to release it, to avoid
* any memory leaks.
*/
private var builtIntoPacket: Boolean = false

/**
* Returns the backing buffer for this cycle.
* @throws IllegalStateException if the buffer has not been allocated yet.
Expand All @@ -175,6 +184,7 @@ public class PlayerInfo internal constructor(
exception,
)
}
this.builtIntoPacket = true
return PlayerInfoPacket(backingBuffer())
}

Expand Down Expand Up @@ -584,6 +594,7 @@ public class PlayerInfo internal constructor(
// Acquire a new buffer with each cycle, in case the previous one isn't fully written out yet
val buffer = allocator.buffer(BUF_CAPACITY, BUF_CAPACITY)
this.buffer = buffer
this.builtIntoPacket = false
return buffer
}

Expand Down Expand Up @@ -642,9 +653,14 @@ public class PlayerInfo internal constructor(
* to stick around for extended periods of time. Any primitive properties will remain untouched.
*/
override fun onDealloc() {
val buffer = this.buffer
if (buffer != null && buffer.refCnt() > 0) {
buffer.release(buffer.refCnt())
// If player info was constructed, but it was not built into a packet object
// it implies the packet is never being written to Netty, which means
// a memory leak is occurring - if that is the case, release the buffer here
if (!builtIntoPacket) {
val buffer = this.buffer
if (buffer != null && buffer.refCnt() > 0) {
buffer.release(buffer.refCnt())
}
}
this.buffer = null
avatar.extendedInfo.reset()
Expand Down

0 comments on commit 5e5cfc2

Please sign in to comment.