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

Expose stoppable.increment and stoppable.decrement #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

snakamura
Copy link
Contributor

I'm using stoppable with WebSocket. Since stoppable itself doesn't handle WebSocket (and it's impossible for stoppable itself to know when it can shutdown), I manually manipulate server._pendingSockets and close connections in my code. But I'm not comfortable depending on private APIs.

This patch exposes two new methods (stoppable.increment and stoppable.decrement) from a server. With these methods, I can tell stoppable from my code when is appropriate to shutdown.

readme.md Outdated
server.stoppable.increment(socket)
```

Increment a counter for a specified socket. This method returns a new counter.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "a new counter" mean? The new "current count" of the socket?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the incremented pending count of the specified socket.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps rephrase to "This method returns the new count." or something like that? Apparently I guessed right, but it really was a guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing it out. Updated.

@snakamura
Copy link
Contributor Author

@hunterloftis Is there any chance for this PR to be merged?

Copy link
Collaborator

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need some tests, but also: can you please explain more about why this change is necessary?

Copy link
Collaborator

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need some tests, but also: can you please explain more about why this change is necessary?

@snakamura
Copy link
Contributor Author

snakamura commented Apr 11, 2018

@boneskull With normal HTTP requests, stoppable can determine whether a server is processing requests or not. I mean, it can think a server starts processing after request event is fired on the server, and stops processing after finish event is fired on a socket. Stoppable can stop the server gracefully while it's not processing any requests by closing its socket.

On the other hand, when it comes to WebSocket, there are no clear definitions when a server is processing. For example, imagine you're designing an application using WebSocket, and decided a server sends two WebSocket messages when it receives one WebSocket message from a client. In this case, you don't want to shutdown the server after it sends the first message but before sending the second one. So it's up to an application if a server can be shutdown gracefully.

To tell stoppable whether it can stop the server gracefully, we need help from an application, hence these new APIs.

@Lewiscowles1986
Copy link

Note: if an application can hang the server unless correctly configured, I'd question the server design. It should be possible (using another codebase) to close the other connections separately, or just force quit the server after n seconds, which this lib says it supports.

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.

4 participants