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

Fix issue #834 by starting WebSocketWorker of the WebSocketServer in the start function #850

Merged
merged 3 commits into from
Feb 10, 2019

Conversation

Andreas-Bur
Copy link
Contributor

Description

I changed the constructor of the WebSocketServer class to only instantiate the WebSocketWorkers but not start them. Now they are started in a foreach loop in the start function.

Related Issue

This fixes the issue #834.

Motivation and Context

Now the WebSocketWorkers are only started when the WebSocketServer is started instead of when it was instantiated. These WebSocketWorkers run in different threads and would keep the program alive even though the WebSocketServer was not running but simply instantiated.

How Has This Been Tested?

All JUnit tests passed and the gist in the issue now properly terminates since the worker threads are never started. I did not see the need for an additional JUnit test for this small fix but would happily provide one if desired.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@marci4
Copy link
Collaborator

marci4 commented Feb 10, 2019

Hello @Andreas-Bur,

Thank you for your pull request.

Could you please add a test for your changes.
Just wanna make sure that it stays that way.

Best regards,
Marcel

@Andreas-Bur
Copy link
Contributor Author

Ok so I've created the test Issue834Test which checks that the the threads are the same before and after instantiating a new WebSocketServer object. I thought this was the cleanest way since the list decoders with the WebSocketWorkers is protected.

By the way I have noticed that not all issue tests are being run in the AllIssueTests class (e.g. Issue847Test). Is this a mistake?

@marci4
Copy link
Collaborator

marci4 commented Feb 10, 2019

Hello @Andreas-Bur,

thank you for your changes.

Yes, that can be the case, mostly since I am now using maven to test it directly.

I put it onto my todo list.

Best regards,
Marcel

@marci4 marci4 merged commit 8a89706 into TooTallNate:master Feb 10, 2019
@marci4 marci4 added this to the Release 1.4.0 milestone Feb 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants