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

Refactor channel closure handling #118

Conversation

SanHalacogluImproving
Copy link

@SanHalacogluImproving SanHalacogluImproving commented Feb 29, 2024

Replaced future cancellations (for noncomplete futures) with exceptional completion using a ClosingException upon channel closure, aligning with Python and Node clients.
Added a mechanism to return a future with a ClosingException for command submissions on already closed clients. Introduced checks to see if the client has been already closed to catch it before submitting new commands.

…command submissions on closed clients now also return a future with a ClosingException Set. (Consistent with Py and Node clients)
@@ -23,6 +23,10 @@ public class ChannelHandler {
protected final Channel channel;
protected final CallbackDispatcher callbackDispatcher;

public boolean isClosed() {
return !channel.isOpen();

Choose a reason for hiding this comment

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

I'm not sure that it is an immediate transition. channel.close is an async operation so you (or user) still may be able to write something to a channel is being closed

Choose a reason for hiding this comment

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

And if a user writes something to the channel before the channel.close() call finishes, we could get in a state where the channel is still open (and will accept a request and add a request to the list - which will never complete).
@SanHalacogluImproving let's change this a private boolean field that saves the state of the channelHandlers.

   private boolean isClosed = false;

    public boolean isClosed() {
        return this.isClosed;
    }

    public ChannelFuture close() {
        this.isClosed = true;
        callbackDispatcher.shutdownGracefully();
        return channel.close();
    }

Copy link

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Try to add IT, for example

future = client.cmd()
client.close()
future.get()

@@ -129,7 +129,14 @@ public void distributeClosingException(String message) {
}

public void shutdownGracefully() {
responses.values().forEach(future -> future.cancel(false));
responses

Choose a reason for hiding this comment

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

interesting spotless formatting here.
Any way to make this cleaner? Like:

        String msg =
                "Operation terminated: The closing process has been initiated for the" + " resource.";
        responses.values().forEach(future -> future.completeExceptionally(new ClosingException(msg)));
        responses.clear();

@@ -23,6 +23,10 @@ public class ChannelHandler {
protected final Channel channel;
protected final CallbackDispatcher callbackDispatcher;

public boolean isClosed() {
return !channel.isOpen();

Choose a reason for hiding this comment

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

And if a user writes something to the channel before the channel.close() call finishes, we could get in a state where the channel is still open (and will accept a request and add a request to the list - which will never complete).
@SanHalacogluImproving let's change this a private boolean field that saves the state of the channelHandlers.

   private boolean isClosed = false;

    public boolean isClosed() {
        return this.isClosed;
    }

    public ChannelFuture close() {
        this.isClosed = true;
        callbackDispatcher.shutdownGracefully();
        return channel.close();
    }

@@ -22,9 +22,10 @@ public class ChannelHandler {

protected final Channel channel;
protected final CallbackDispatcher callbackDispatcher;
private boolean isClosed = false;

Choose a reason for hiding this comment

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

Use AtomicBoolean which is thread safe

Choose a reason for hiding this comment

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

Note: except that the variable is bi-directional. It doesn't matter if multiple threads set isClosed to true, it will be true.

@Yury-Fridlyand
Copy link

CI is red

@SanHalacogluImproving SanHalacogluImproving changed the title Java: Handle ClosingError channel.close() Refactor channel closure handling Mar 4, 2024
@SanHalacogluImproving SanHalacogluImproving merged commit be50490 into java/integ_SanH_handle_closing_exception Mar 4, 2024
10 checks passed
SanHalacogluImproving added a commit that referenced this pull request Mar 4, 2024
* Ensure resources complete with Closing Error when client closes. New command submissions on closed clients now also return a future with a ClosingException Set. (Consistent with Py and Node clients)

* Added a private boolean that saves the state of channel handler.

* Changed Boolean to Atomic Boolean added IT tests and modified UT tests.

* Fix IT
SanHalacogluImproving added a commit that referenced this pull request Mar 5, 2024
* Refactor channel closure handling (#118)

* Ensure resources complete with Closing Error when client closes. New command submissions on closed clients now also return a future with a ClosingException Set. (Consistent with Py and Node clients)

* Added a private boolean that saves the state of channel handler.

* Changed Boolean to Atomic Boolean added IT tests and modified UT tests.

* Fix IT

* Minor refactor for ClientTests.
@SanHalacogluImproving SanHalacogluImproving deleted the java/dev_SanH_handle_closing_exception branch March 19, 2024 18:01
cyip10 pushed a commit that referenced this pull request Jun 24, 2024
* Refactor channel closure handling (#118)

* Ensure resources complete with Closing Error when client closes. New command submissions on closed clients now also return a future with a ClosingException Set. (Consistent with Py and Node clients)

* Added a private boolean that saves the state of channel handler.

* Changed Boolean to Atomic Boolean added IT tests and modified UT tests.

* Fix IT

* Minor refactor for ClientTests.
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.

3 participants