search

ebarlas / microhttp

Fast, scalable, self-contained, single-threaded Java web server
MIT License
539 stars 54 forks source link

Expose a method for getting an OptionsBuilder on the Options #20

Closed filiphr closed 1 year ago

filiphr commented 1 year ago

This iterates on #17 by exposing the builder through Options#builder() it makes it a bit easier for discovery and less classes that should be imported when creating a Options

ebarlas commented 1 year ago

Thanks for the contribution, @filiphr!

I prefer the builder() method name as well.

And socket closures were indeed missing in event loop termination. However, you only targeted the passive server socket in EventLoop and not the active socket connections in ConnectionEventLoop. I think we should address the gap in both places.

Also, I think failsafe closures should happen independently (separate try-catch) for the selector and socekt.

filiphr commented 1 year ago

Also, I think failsafe closures should happen independently (separate try-catch) for the selector and socekt.

The closures are a mistake in this PR. I thought I removed that. I've created PR #21 for the closures.

However, you only targeted the passive server socket in EventLoop and not the active socket connections in ConnectionEventLoop. I think we should address the gap in both places.

I had a feeling about this and even with the closures I did in PR #21 things kept lingering when I was using it in my tests. However, I didn't know how to write a test to reproduce this (my test setup where I can reproduce it is rather complicated). In any case, let's move the closure discussion in PR #21, sorry for mixing it up