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

Rethink out-of-memory handling in IStorageChannels #338

Closed
gavin-norman-sociomantic opened this issue Jun 25, 2018 · 9 comments
Closed

Rethink out-of-memory handling in IStorageChannels #338

gavin-norman-sociomantic opened this issue Jun 25, 2018 · 9 comments
Assignees
Milestone

Comments

@gavin-norman-sociomantic
Copy link

gavin-norman-sociomantic commented Jun 25, 2018

IStorageChannels currently has some kind of weird support for catching out-of-memory errors, and setting a flag to disallow creation of more channels (see here). OutOfMemoryException is deprecated in ocean, and is an alias to an Error in D2. Catching Errors is not recommended.

We need to rethink this.

@gavin-norman-sociomantic
Copy link
Author

The more robust approach, when creating a new channel, would be to check whether there's memory space available for the channel before attempting to create it. I'm not sure how feasible this is, though.

@gavin-norman-sociomantic
Copy link
Author

Ok I've done some local tests with a DMQ node, allocating and filling large channels.

  • The current OOM behaviour in D1 works. (If allocating a channel causes OOM, the node will disallow creation of further channels.)
  • Simply removing the OOM try/catch block, the OOM exception is caught in the event loop. (Presumably in D2 this would cause the application to exit.)

@gavin-norman-sociomantic
Copy link
Author

gavin-norman-sociomantic commented Dec 21, 2018

Here's a rethought:

  • We remove the ability for channels to be created ad hoc when a request for a new channel comes in.
  • Instead, creating channels is only possible via a unix socket command (i.e. by the node administrators).
  • This way, the scope for the number of channels to grow without bounds (e.g. due to a client bug) is removed.

@gavin-norman-sociomantic
Copy link
Author

Of course, channel creation isn't the only way we can get into an OOM situation in a node. In the DHT, for example, every record that's added causes a memory allocation that could fail due to OOM.

@gavin-norman-sociomantic
Copy link
Author

Instead, creating channels is only possible via a unix socket command (i.e. by the node administrators).

This would be a slight hassle, as the command would need to be run on every node. But as this would only ever be done in careful coordination with an app reading or writing a new channel, I think the risk is minimal.

@gavin-norman-sociomantic
Copy link
Author

So what I'm proposing is a 2-step fix here:

  1. Remove the handling of OutOfMemoryException. If OOM occurs in IStorageChannels, the thrown Error will cause the program to crash (just like an OOM in any other code location will).
  2. Remove the ability to create channels ad hoc. Replace this with a unix socket command to create a new channel.

Step 1 fixes the basic problem that this issue describes (OutOfMemoryException being deprecated).
Step 2 fixes a node vulnerability that whereby a malicious or buggy client could crash nodes by relentlessly creating new channels.

@gavin-norman-sociomantic
Copy link
Author

Remove the ability to create channels ad hoc.

All requests (both legacy and neo) would be adapted to return the Error status code, if the specified channel does not exist.

@gavin-norman-sociomantic
Copy link
Author

gavin-norman-sociomantic commented Jan 3, 2019

The only problem with this suggestion, I think, is that it removes the capability for channels to be created "programmatically" (i.e. channel names generated at runtime by an application). We've never needed this capability, but we'd be in trouble if we ever did need it, after having removed it.

gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 3, 2019
Previously, the only way to create a new channel from the outside was the
`getCreate` method. As part of sociomantic-tsunami#338, we want to disallow ad hoc channel
creation by request handlers. `getCreate` is to be deprecated, request
handlers should be updated to use `opIn_r` to check for a channel's
existence, and `create` is to be called via a unix socket command handler
specifically to create new channels.
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 3, 2019
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 3, 2019
We plan to remove the capacity for the ad hoc creation of new channels in
request handlers. Channels should instead be created via the newly added
unix socket command.


Part of sociomantic-tsunami#338.
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 3, 2019
`OutOfMemoryException` is deprecated. In D2, it is an `Error`, so should
not be caught.

Fixes sociomantic-tsunami#338.
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 8, 2019
Previously, the only way to create a new channel from the outside was the
`getCreate` method. As part of sociomantic-tsunami#338, we want to disallow ad hoc channel
creation by request handlers. `getCreate` is to be deprecated, request
handlers should be updated to use `opIn_r` to check for a channel's
existence, and `create` is to be called via a unix socket command handler
specifically to create new channels.
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 8, 2019
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 8, 2019
We plan to remove the capacity for the ad hoc creation of new channels in
request handlers. Channels should instead be created via the newly added
unix socket command.


Part of sociomantic-tsunami#338.
gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 8, 2019
`OutOfMemoryException` is deprecated. In D2, it is an `Error`, so should
not be caught.

Fixes sociomantic-tsunami#338.
@gavin-norman-sociomantic
Copy link
Author

Another problem came up in verbal discussion: adding new nodes to a DHT becomes hassle. (You'd need to add all extant channels.)

The conclusion of our discussion was to just remove the OOM exception handling and allow the node to crash in this situation. The burden of avoiding OOM is then on the node maintainers (which is the status quo anyway).

gavin-norman-sociomantic pushed a commit to gavin-norman-sociomantic/swarm that referenced this issue Jan 15, 2019
`OutOfMemoryException` is deprecated. In D2, it is an `Error`, so should
not be caught.

Fixes sociomantic-tsunami#338.
gavin-norman-sociomantic pushed a commit that referenced this issue Jan 17, 2019
`OutOfMemoryException` is deprecated. In D2, it is an `Error`, so should
not be caught.

Fixes #338.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants