You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "crossoverJie (via GitHub)" <gi...@apache.org> on 2023/07/04 03:10:46 UTC
[GitHub] [pulsar-client-go] crossoverJie opened a new pull request, #1046: [Fix] Multiple calls to client.Close causes panic
crossoverJie opened a new pull request, #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046
Fixes #1026
### Motivation
Multiple calls to `client.Close` causes panic.
### Modifications
Add `closeOnce sync.Once`
### Verifying this change
- [x] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Shoothzj commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1619421145
This PR enables the 'close' method to be re-entrant. However, I'm not quite seeing the value in making the 'close' method re-entrant. Could you provide any scenarios or use cases where this would be necessary?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] Shoothzj commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "Shoothzj (via GitHub)" <gi...@apache.org>.
Shoothzj commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1712540130
I believe this modification is feasible. Regardless of whether or not it supports reentrancy, and whether reentrancy supports concurrency, we can maintain consistent behavior across the entire code repository.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] tisonkun commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1712701370
Thanks for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] YanshuoH commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "YanshuoH (via GitHub)" <gi...@apache.org>.
YanshuoH commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1705250728
> This PR enables the 'close' method to be re-entrant. However, I'm not quite seeing the value in making the 'close' method re-entrant. Could you provide any scenarios or use cases where this would be necessary?
I might add a scenario to this improvement.
For example, our app may instantiate multiple instances that implement `io.Closer`, we might do something like:
- create a server that will create multiple rpc instances (including pulsar.Client) inside.
- `server.Run` func that has a `defer cleanup()` within. That `cleanup` would try to close the rpc instances.
- a `defer server.Close()` func that calls `cleanup` underling that will be used in the main file.
In Go, we expect `Close` that can be called multiple times, so the above workflow is designed to make sure all rpc instances are closed properly without leaving any openfd or occupying other resources.
That is the opportunity that I've found out https://github.com/apache/pulsar-client-go/issues/1026.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] RobertIndie commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "RobertIndie (via GitHub)" <gi...@apache.org>.
RobertIndie commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1709784913
@tisonkun @Shoothzj Could you take a look at this? Does the explanation make sense to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] tisonkun merged pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] crossoverJie commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "crossoverJie (via GitHub)" <gi...@apache.org>.
crossoverJie commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1619426622
This is just defensive programming, the consumer's close logic also has similar usage.
https://github.com/apache/pulsar-client-go/blob/5f8df2782251226b63b2dad9f4e87fd4bbd31ef5/pulsar/consumer_multitopic.go#L256-L270
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] crossoverJie commented on pull request #1046: [Fix] Multiple calls to client.Close causes panic
Posted by "crossoverJie (via GitHub)" <gi...@apache.org>.
crossoverJie commented on PR #1046:
URL: https://github.com/apache/pulsar-client-go/pull/1046#issuecomment-1619852021
> What if we cause the internal closes idempotent instead of hacky wrapping an `Once` outer?
https://github.com/apache/pulsar/blob/a3bca685a4ef0916462228e035f77fdbf295653d/pulsar-client/src/main/java/org/apache/pulsar/client/impl/PulsarClientImpl.java#L735-L739
I looked at the Java SDK's close logic, it uses an internal state to maintain the client state, and the `Once` principle used here is similar.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org