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

Document how to have read timeout be larger than idle timeout #425

Open
RichieSams opened this issue Nov 29, 2023 · 3 comments
Open

Document how to have read timeout be larger than idle timeout #425

RichieSams opened this issue Nov 29, 2023 · 3 comments
Labels
Milestone

Comments

@RichieSams
Copy link

Hi! Thanks for this excellent library.

I have a use case where I want the server to wait up to 10 seconds for the client to start sending data. If the server doesn't get a new Reader within that time, it closes the connection and bails.

However, if the client does start sending data, then they have a much longer time limit to finish sending. Say 2 minutes. As far as I can tell, there's not a good way to accomplish this with the current API design.

I saw this issue: #87

Which works IFF the idle timeout is larger than the read timeout.

This is because the Conn.msgReader holds on to the context passed in during Reader() Example:

ctx, cancel := context.WithTimeout(rootCtx, 10 *second)
defer cancel()

_, reader, err := ws.Reader(ctx)
if err != nil {
	// Err handling....
	log.Error(err)
	return
}

// If reading data takes longer than 10 seconds, the timeout above will fire, and the context will be cancelled
// Killing the connnection
data, err := io.ReadAll(reader)
if err != nil {
	// Err handling....
	log.Error(err)
	return
}

I can potentially work around this. Instead of using context.WithTimeout, I could just use context.WithCancel. Then have a time.AfterFunc(), which uses atomics to check if we got the reader already. In which case, don't cancel. Example:

ctx, cancel := context.WithCancel(rootCtx)
defer cancel()

gotReader := atomic.Bool{}

time.AfterFunc(10*time.Second, func() {
	if !gotReader.Load() {
		cancel()
	}
})

_, reader, err := ws.Reader(ctx)
if err != nil {
	// Err handling....
	log.Error(err)
	return
}
gotReader.Store(true)

time.AfterFunc(2*time.Minute, func() {
	cancel()
})

data, err := io.ReadAll(reader)
if err != nil {
	// Err handling....
	log.Error(err)
	return
}

I'm not sure what the best way to modify the API would be. You're unfortunately stuck with the io.Reader interface, without the ability to add a context. Thoughts?

@nhooyr
Copy link
Owner

nhooyr commented Nov 29, 2023

Interesting, I've never seen this use case before. But yes, you've got the idea right, you'd have to use time.AfterFunc with context.WithCancel. You wouldn't need to use atomics though, just cancel the timer after you get a reader. See the return value of time.AfterFunc.

Unfortunately I cannot suggest anything better nor do I think there's any good way to modify the API to accommodate this. Perhaps you might benefit from using the net.Conn adapter and use Set*Deadline to set the deadline more explicitly as you need to.

@nhooyr nhooyr closed this as completed Nov 29, 2023
@RichieSams
Copy link
Author

Thanks :)

@nhooyr
Copy link
Owner

nhooyr commented Mar 7, 2024

Let's add an example for this though for sure.

@nhooyr nhooyr reopened this Mar 7, 2024
@nhooyr nhooyr added the docs label Mar 7, 2024
@nhooyr nhooyr added this to the v1.9.0 milestone Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants