bug: Rate limiting is at the back of the send queue

[TheDaemoness]

🌧 Describe the problem

girc's rate limit is checked before pushing a message onto the send queue tx.
This means if the connection stalls for long enough for tx to fill up with 6 messages or more while otherwise obeying the rate limit, it will likely flood itself off the moment the connection recovers.

Additionally, girc does not provide a mechanism by which a user of girc can add their own rate limiting in the correct place in sendLoop.

⛅ Expected behavior

Any send rate limiting should be checked at the point where messages are popped off the send queue. Additionally, it should be possible for users of girc to introduce their own mechanisms for rate limiting, as girc's rate limit is hard-coded to be more generous than recommendations from RFC1459 (TODO: future issue).

🔄 Minimal reproduction

No response

💠 Version: girc

any

🖥 Version: Operating system

other

⚙ Additional context

Issue is architectural and seems to exist for all versions of girc with rate limiting.

Rate limit occurs here:

girc/conn.go

Lines 445 to 463 in e3e8859

	if !c.Config.AllowFlood {
	c.mu.RLock()
	// Drop the event early as we're disconnected, this way we don't have to wait
	// the (potentially long) rate limit delay before dropping.
	if c.conn == nil {
	c.debugLogEvent(e, true)
	c.mu.RUnlock()
	return
	}
	c.conn.mu.Lock()
	delay = c.conn.rate(e.Len())
	c.conn.mu.Unlock()
	c.mu.RUnlock()
	}
	<-time.After(delay)
	c.write(e)

write pushes to tx here:

girc/conn.go

Line 484 in e3e8859

case c.tx <- event:

tx is popped and written in sendLoop here:

girc/conn.go

Lines 514 to 544 in e3e8859

	case event := <-c.tx:
	// Check if tags exist on the event. If they do, and message-tags
	// isn't a supported capability, remove them from the event.
	if event.Tags != nil {
	c.state.RLock()
	var in bool
	for i := 0; i < len(c.state.enabledCap); i++ {
	if _, ok := c.state.enabledCap["message-tags"]; ok {
	in = true
	break
	}
	}
	c.state.RUnlock()
	if !in {
	event.Tags = Tags{}
	}
	}
	c.debugLogEvent(event, false)
	c.conn.mu.Lock()
	c.conn.lastWrite = time.Now()
	if event.Command != PING && event.Command != PONG && event.Command != WHO {
	c.conn.lastActive = c.conn.lastWrite
	}
	c.conn.mu.Unlock()
	// Write the raw line.
	_, err = c.conn.io.Write(event.Bytes())

🤝 Requirements

• I believe the problem I'm facing is a bug, and is not intended behavior. Post here if you're not sure.
• I have confirmed that someone else has not submitted a similar bug report.