Add explicit handling for max clients connected to the server

[JyotinderSingh]

The below code snippet is used inside async_tcp.go, however, it cannot effectively limit the maximum number of clients connected to the server. We need to add an explicit check for the same.

if err := syscall.Listen(s.serverFD, s.maxClients); err != nil {
		return err
	}

Context: https://github.com/DiceDB/dice/pull/410#discussion_r1731693923

[cskarthik7]

Can I pick this up?

[TheRanomial]

I would like to work on this one

[ayush571995]

https://github.com/DiceDB/dice/pull/426/files @JyotinderSingh Can you have a look ?

[evoxtorm]

@ ayush571995 I also have the same code as you, but how did you test it?

[ayush571995]

@ ayush571995 I also have the same code as you, but how did you test it?

In local for testing i changed the max connection limit to 1 Umm using dice cli from terminal I created multiple connections and got the failure when exceeded limit

[evoxtorm]

@ayush571995 Thanks, I was writing a test case, I tried something like this, and I don't know why it failed for me. Am I missing something here?

connections := make([]net.Conn, 0, config.ServerMaxClients+1)

	for i := 0; i < config.ServerMaxClients+1; i++ {
		conn := getLocalConnection()
		if conn != nil {
			connections = append(connections, conn)
		}
		time.Sleep(100 * time.Millisecond) 
	}

	// t.Log(len(connections), " This is log", config.ServerMaxClients)
	assert.Equal(t, config.ServerMaxClients, len(connections), "Should only be able to open maxClients connections")

[JyotinderSingh]

Since @ayush571995 already created a draft PR for this, assigning this to him.

[arpitbbhayani]

Hello @ayush571995,

There has been no activity on this issue for the past 5 days. It would be awesome if you keep posting updates to this issue so that we know you are actively working on it.

We are really eager to close this issue at the earliest, hence if we continue to see the inactivity, we will have to reassign the issue to someone else. We are doing this to ensure that the project maintains its momentum and others are not blocked on this work.

Just drop a comment with the current status of the work or share any issues you are facing. We can always chip in to help you out.

Thanks again.

[ayush571995]

Hey @arpitbbhayani @JyotinderSingh Sorry I missed this . Will push the changes today

[ayush571995]

@arpitbbhayani @JyotinderSingh I was writing a test case for this integration probably but need to discuss one thing about the changes I would like to have a small discussion if possible ?

Question: This test needs to run in isolation and we don't have that setup infra as of now context: I have the changes ready but when i execute the test as part of the test suite it's not able to accquire max connections as some tests have them and it fails

[JyotinderSingh]

@arpitbbhayani @JyotinderSingh

I was writing a test case for this integration probably but need to discuss one thing about the changes

I would like to have a small discussion if possible ?

Question:

This test needs to run in isolation and we don't have that setup infra as of now

context:

I have the changes ready but when i execute the test as part of the test suite it's not able to accquire max connections as some tests have them and it fails

Apologies for the late reply, missed this comment. We now have a server directory under integration_tests/. You can add a test there, the tests in this directory do not launch a default server instance. You have more control over the server configuration and testing there. Please take a look if you can make it work there.

[ayush571995]

Folks, Just a nudge here as well. The pr is ready since 15th Sept for review

[lucifercr07]

@ayush571995 can you please resolve conflicts and review comments on PR

[lucifercr07]

Closing, merged as part of https://github.com/DiceDB/dice/pull/426