search

SherClockHolmes / webpush-go

Web Push API Encryption with VAPID support.
MIT License
312 stars 67 forks source link

Make it clear that the caller is responsible for closing the body #26

Closed pennersr closed 4 years ago

pennersr commented 5 years ago

I am using webpush-go over at https://gitlab.com/pennersr/shove -- thanks! One thing that I initially overlooked was the fact that SendNotification() returns an http.Response, for which the following holds:

If the returned error is nil, the Response will contain a non-nil Body which the user is expected to close. If the Body is not both read to EOF and closed, the Client's underlying RoundTripper (typically Transport) may not be able to re-use a persistent TCP connection to the server for a subsequent "keep-alive" request.

The README/example contains this (it ignores the response):

// Send Notification
_, err := webpush.SendNotification(

So the fact that the caller is responsible for handling the caveat mentioned above is not very obvious.

SherClockHolmes commented 5 years ago

@pennersr Are you recommending a comment in the README? I'm not sure if it's within the scope of this library to remind people of Golang's http.Response behavior. I could potentially add a wiki article with this caveat if you think that will help.

pennersr commented 5 years ago

Even if you are aware of Golang's http.Response behavior, given the example/README it is simply not very obvious that we're dealing with such a struct here. The example nicely documents // TODO: Handle error, I would suggest adding a similar TODO for the http.Response.