Skip to content

net/server: improve error for truthy handler and type :datagram#1615

Merged
bakpakin merged 1 commit intojanet-lang:masterfrom
ifreund:net-server-datagram
Aug 2, 2025
Merged

net/server: improve error for truthy handler and type :datagram#1615
bakpakin merged 1 commit intojanet-lang:masterfrom
ifreund:net-server-datagram

Conversation

@ifreund
Copy link
Copy Markdown
Contributor

@ifreund ifreund commented Jul 17, 2025

Since it is invalid to call accept on a datagram socket, net/server always errors if handler is truthy and type is :datagram.

Tweak the implementation to give a better error message in this case.

References: #1614

@ifreund ifreund force-pushed the net-server-datagram branch from fe2d454 to 92fc152 Compare July 17, 2025 11:27
@sogaiu
Copy link
Copy Markdown
Contributor

sogaiu commented Jul 17, 2025

Thanks for the PR!

If type is nil, net/listen behaves as if type is :stream, right?

Do we want to handle things a bit differently in the case for type being nil and handler is not nil?

Right now it looks to me like the case form might just skip doing anything with handler if type is nil:

(case type
  :stream (if handler
            (ev/go (fn [] (net/accept-loop s handler))))
  :datagram (if handler
              (error "handler not supported for :datagram servers")))

May be we can use (default type :stream) or something earlier in the code? Although I suppose that relies on net/listen's current behavior...may be that's not a problem.

@ifreund ifreund force-pushed the net-server-datagram branch from 92fc152 to 953efc4 Compare July 17, 2025 11:44
@ifreund
Copy link
Copy Markdown
Contributor Author

ifreund commented Jul 17, 2025

Good catch @sogaiu! I think the (default type :stream) approach is fine. It doesn't even really rely on the behavior of net/listen to function as documented, the only downside is that it could become inconsistent with net/listen if the default type there is changed. That doesn't seem likely to ever happen though in my humble opinion :)

@ifreund ifreund force-pushed the net-server-datagram branch from 953efc4 to 0f04372 Compare July 17, 2025 15:51
@ifreund
Copy link
Copy Markdown
Contributor Author

ifreund commented Jul 17, 2025

The newest revision avoids using default and also avoids binding a socket at all if the arguments are invalid.

@sogaiu
Copy link
Copy Markdown
Contributor

sogaiu commented Jul 18, 2025

Just a quick note.

Don't know if it would be an improvement, but perhaps an alternative to this expression:

    (if (and (= type :datagram) handler)
      (error "handler not supported for :datagram servers"))

might be to use assert?

Will think on it a bit more.

Since it is invalid to call accept on a datagram socket, net/server
always errors if handler is truthy and type is :datagram.

Add an assert to give a better error message in this case and clarify
the documentation.

References: janet-lang#1614
@ifreund ifreund force-pushed the net-server-datagram branch from 0f04372 to f974c06 Compare July 18, 2025 07:58
@ifreund
Copy link
Copy Markdown
Contributor Author

ifreund commented Jul 18, 2025

I think I do like the assert a bit more, perhaps this is finally the final version of this patch :)

@sogaiu
Copy link
Copy Markdown
Contributor

sogaiu commented Jul 18, 2025

LGTM

Copy link
Copy Markdown
Member

@pepe pepe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bakpakin bakpakin merged commit e34a854 into janet-lang:master Aug 2, 2025
12 checks passed
@ifreund ifreund deleted the net-server-datagram branch August 4, 2025 07:27
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.

4 participants