Skip to content

Conversation

normanmaurer
Copy link
Member

Motivation:

We need to guard against re-entrance of close0. This can happen at the moment for two reasons:

  1. A user adds a callback that calls close() again when the promise failed because socket.close() throws.
  2. close() is called because Selector.deregister(...) throws and the user calls close() in errorCaught(...).

Modifications:

  • Ensure we only notify the promise once we updated the internal state of the Channel and also failed all pending writes. This way we "fast-return" in close0(...) when called again.
  • Just ignore errors happending because of selector.deregister(...)

Result:

No more re-entrance issues in close0

Motivation:

We need to guard against re-entrance of close0. This can happen at the moment for two reasons:

1) A user adds a callback that calls close() again when the promise failed because socket.close() throws.
2) close() is called because Selector.deregister(...) throws and the user calls close() in errorCaught(...).

Modifications:

- Ensure we only notify the promise once we updated the internal state of the Channel and also failed all pending writes. This way we "fast-return" in close0(...) when called again.
- Just ignore errors happending because of selector.deregister(...)

Result:

No more re-entrance issues in close0
@normanmaurer normanmaurer requested review from weissi and Lukasa March 26, 2018 18:38
@normanmaurer
Copy link
Member Author

I think the only way how I could test it partly would be if I make BaseSocket.close() non final and override it. WDYT ?

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

needs a test and will also conflict with #220

@normanmaurer
Copy link
Member Author

@weissi yep... Thats why I asked if anyone has a better idea for a test ;)

@weissi
Copy link
Member

weissi commented Mar 27, 2018

@normanmaurer sorry, didn't spot that. I think making BaseSocket.close non-final is okay. We already have much more performance sensitive methods like read non-final so that should be cool.

@weissi weissi added the 🔨 semver/patch No public API change. label Mar 27, 2018
@Lukasa
Copy link
Contributor

Lukasa commented Mar 28, 2018

Non-final BaseSocket works for me.

@Lukasa
Copy link
Contributor

Lukasa commented Apr 12, 2018

IMO this is superseded by #220, so we could probably close this.

@normanmaurer
Copy link
Member Author

Yep let me close... Thanks

@normanmaurer normanmaurer deleted the close0_reentrance branch April 12, 2018 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants