Events handlers are cleared on .end()

janispritzkau / rcon-client

A simple and modern RCON client made to work with Minecraft

64 stars 17 forks source link

Events handlers are cleared on .end() #4

Closed Hornwitser closed 4 years ago

Hornwitser commented 5 years ago

When closing the client using the .end() on the the Rcon class all the event listeners on the socket is cleared, this causes two problems.

It's not possible to wait for the socket to properly close as the end event is never emitted.
Errors during the close operation is not handled. For example if the game server connected to responds with RST instead of FIN you get the following unhelpful error message printed to the console:
```
events.js:174
  throw er; // Unhandled 'error' event
  ^
```

Error: read ECONNRESET at TCP.onStreamRead (internal/stream_base_commons.js:111:27) Emitted 'error' event at: at emitErrorNT (internal/streams/destroy.js:91:8) at emitErrorAndCloseNT (internal/streams/destroy.js:59:3) at process._tickCallback (internal/process/next_tick.js:63:19)

janispritzkau commented 5 years ago

Hmm. It was kind of needed if you called rcon.end() synchronously before rcon.connect(). If I remove it, you would need to wait until the socket is closed before you reconnect.

I probably forgot to emit the end event on rcon.end().

I'm gonna think about it.

Hornwitser commented 5 years ago

If you try to connect but are currently in the process of disconnecting the correct action to take is to either throw an error or wait until the disconnect completes.

If you try to connect while not yet disconnected you enter the path of insanity where you have multiple sockets open and are receiving the close event or error for one after the connect event of another. There is no way to properly handle that.

janispritzkau commented 5 years ago

Which option do you think is better? Throwing errors does require to make sure that you're not in the process of connecting or disconnecting. With the other option you can just call .connect, no matter if you're in the state of connecting or not, and just expect the Rcon instance to be ready to use when the Promise resolves.

janispritzkau commented 5 years ago

The first one is definitely easier to implement.

Hornwitser commented 5 years ago

I think throwing an error is better as calling .connect when you're already connected or in the process of disconnecting sounds like an error in the application to me. But it doesn't matter much to my use case as I don't expect the RCON connection to drop since it's over the loopback interface. What's more important to me is that I get to know when the RCON connection is properly closed so I can wait with terminating the server.

janispritzkau commented 5 years ago

Can you test the current master branch? I haven't published it on npm yet.

Hornwitser commented 5 years ago

Current master appears to work, yes. It passes error during rcon.end() to the error callback. And it waits until after the connection is closed before returning from rcon.end().

axi92 commented 4 years ago

I think I had the same problem with the game Ark Survival Evolved. #7

janispritzkau commented 4 years ago

I think this issue is already fixed, so I'm closing it.