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

Improve handling of closing gates/spaces and connection lost #34

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

Bogoe
Copy link
Contributor

@Bogoe Bogoe commented Jan 30, 2023

This pull request aims to fix the following issues in relation to closing gates/spaces or if one loses connection when using KEEP connections:

  1. OLD BEHAVIOR: When SpaceRepository.shutdown() had been called, the SpaceRepository would in some cases still try to execute new tasks (listen for messages from a client or try to send a response) which throw RejectedExecutionException's.
    NEW BEHAVIOR: Tasks are no longer enqueued after SpaceRepository.shutdown() has been called.
  2. OLD BEHAVIOR: After a RemoteSpace was closed (or connection was lost in some way), all threads that earlier had called get, query, put, ect. on a RemoteSpace would become stuck (they could be interrupted), because they never got an answer (no InterruptedException would be triggered).
    NEW BEHAVIOR: When a RemoteSpace is closed, all waiting threads are now interrupted - they throw an InterruptedException.
  3. OLD BEHAVIOR: Calling get, query, put, ect. on a RemoteSpace AFTER it is closed would make the calling thread stuck (it could be interrupted).
    NEW BEHAVIOR: An InterruptedException is thrown.
  4. OLD BEHAVIOR: When RemoteSpace.close() was called, the calling thread would become stuck, unless the thread responsible for receiving messages (KeepClientGate.inboxHandlingMethod()) got a message. The reason is that BufferedReader.readLine() takes a lock that is also needed for BufferedReader.close().
    NEW BEHAVIOR: The underlying socket is now closed first, which effectively also closes the BufferReader, so a RemoteSpace now can be closed without getting stuck.
  5. OLD BEHAVIOR: If a RemoteSpace was connected to a SpaceRepository, then calling SpaceRepository.shutdown() would make the thread responsible for receiving messages (KeepClientGate.inboxHandlingMethod()) enter an infinite loop (due to the message was null) - effectively wasting one CPU core on nothing.
    NEW BEHAVIOR: The thread now closes the gate automatically in this case, since a message being null means EOF (socket closed).

Very likely fixes issue #12.

I added a file TestCloseKeepConnection.java with six tests to test some of the things above. Each test runs one thread as server and one thread as client, concurrently (they both create even more threads). The tests work most of the time, but also fails too much to be called stable. One problem is that sometimes, some of the tests fail because there are more threads running after the test is finished than there where when it started. Another problem is that the tests sometimes get stuck (runs for ever), because the threads that are joined never terminate. I did not set a timeout, because that would not help making them stable. The second problem only seems to appear when running the tests via Maven in Eclipse, while the first problem is independent of how they are ran. Therefore, these tests are NOT ran when running all tests, because there might still be some concurrency issue.

Changes in:

  • SpaceRepository.java covers point 1.
  • KeepClientGate.java covers point 2, 3, 4 and 5.
  • Rendezvous.java covers point 2.
  • TestRemoteSpace.java one test was commented out (it never failed when I ran it).
  • TestjSonMessageSerialization.java removed debug print.
  • TestCloseKeepConnection.java new tests that are NOT included when running the tests, since they are unstable.

@albertolluch albertolluch merged commit 5e07862 into pSpaces:master Jan 30, 2023
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.

2 participants