Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support non-blocking read #688

Closed
wants to merge 0 commits into from
Closed

Conversation

iluozhiqiang
Copy link

Fixes #481

In the case of massive connections, we hope to use a single goroutine to cooperate with epoll to realize the message reading of the connection, instead of starting a goroutine separately for each connection.
Because goroutines themselves also have overhead, the scheduling performance of goroutines will also decrease linearly when there are massive connections.

Summary of Changes

  1. add allowReadControlMessages field on Conn
  2. add AllowReadControlMessages method on Conn, the method set true of allowReadControlMessages on Conn
  3. Change NextReader method , if allowReadControlMessages is true then return control message

PS: Make sure your PR includes/updates tests! If you need help with this part, just ask!

@iluozhiqiang iluozhiqiang changed the title add AllowReadControlMessages field, the field using support non-blocking read Support non-blocking read May 13, 2021
@ghost
Copy link

ghost commented May 13, 2021

This PR breaks out of the loop in NextReader after processing control frames.

NextReader returns noframe, nil, nil for PING and PONG messages. Because ReadMessage and ReadJSON expect a non-nil reader on success, those functions will panic on a control message.

The PR does not handle the case where data remains in the bufio.Reader. If two messages were slurped up in to the bufio.Reader, the second message will not be handled until a third messages is received. The third message will not be received until the fourth message and so on.

Given the above, the feature should be implemented something like this:

if c.allowReadControlMessages  && c.bf.Len() == 0 {
     return noframe, nil, ErrNoData
}

where ErrNoData is a new exported error value.

AllowReadControlMessages is not a good name for the public API. Control messages are always allowed. The name should somehow indicate that the feature is about returning from NextReader on no data.

The function that enables the feature should take a bool argument so that the application can turn the feature on and off.

@ghost
Copy link

ghost commented May 13, 2021

A read buffer pool is the next obvious change for the non-blocking scenario. Any PR to support the non-blocking scenario should be a complete solution. We should avoid littering the API with partial attempts at fixing the problem.

There is active discussion on non-blocking read in the standard library. The standard library solution for non-blocking read may have an impact on how the feature is exposed here. If network connections invoke a callback when data is ready, this package should probably do the same. On the other hand, nothing has happened in the five years since the issue was opened.

This PR is missing tests. All features should have a test. Hold off on that until the other issues are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support evented read
1 participant